Coding Conventions

1. Langugage and License

Saros is licensed under GPLv2.
All 3rd party code that has not been written by a Saros team member (i.e. an open-source contributor, a student or a member of the AG Software Engineering at the Freie Universität Berlin) is kept in a separate source folder named ext-src.

All code (i.e. identifiers) and comments are written in American English.

2. Project Structure

The project is hosted on Sourceforge and replicated on GitHub. The corresponding Git repositories are read-only. All code changes go through our review system Gerrit.

The Saros project's code is split as follows:

• IDE-independent source code and libraries:
  • de.fu_berlin.inf.dpp.core
• The Eclipse world: Saros/E
  • de.fu_berlin.inf.dpp contains the Eclipse-specific sources and libraries. Saros/E is the combination of this project and the core.
  • de.fu_berlin.inf.dpp.whiteboard contains the Saros whiteboard plugin, which is only available for and depends on Saros/E.
  • de.fu_berlin.inf.dpp.feature is necessary for creating the Saros/E plugin
  • de.fu_berlin.inf.dpp.update is necessary for creating the update-site for installing or updating Saros/E
  • de.fu_berlin.inf.dpp.update_beta is the update-site used for User Acceptance Test during a release process.
• The IntelliJ world: Saros/I
  • de.fu_berlin.inf.dpp.intellij contains the IntelliJ-specific sources and libraries. Saros/I is the combination of this project and the core.
  • de.fu_berlin.inf.dpp.swt_plugin contains Eclipse's SWT libraries to be distributed as a separate plugin along with the intellij project.

3. Naming Conventions

• This section is mainly based on the Code Conventions for the Java TM Programming Language, Chapter 9
• Differences and additional conventions
  • Non-Listener interfaces should be preceded by an I
    • e.g: IProject, IPreferenceStore
  • Listener interfaces should use the name of their corresponding class and add Listener to it
    • e.g. MouseListener
• All test case classes must end with Test. e.g HelloWorldTest
• A test suite classes must contain the character sequence TestSuite
• Every test class that is used for White-Box testing must be declared in the same package , e.g foo.bar.HelloWorld -> foo.bar.HelloWorldTest
• STF test cases must be put in any subpackage of de.fu_berlin.inf.dpp.stf.test, e.g de.fu_berlin.inf.dpp.stf.test.account.AccountPreferencePageTest

4. Structuring your code

See here.

5. Writing Java Classes

See here.

6. JavaDoc Tags

• The following JavaDoc tags should be used for highlighting important aspects of a method or a class:
  • @ui or @swing or @swt - This method needs to be called from the UI thread, otherwise an exception might occur.
  • @nonBlocking - This method does return before finishing its activities. If there is at least one method is a class which is non-blocking it is highly recommended to put @blocking to all other methods to help users recognize that blocking behavior is important for this class
  • @blocking - This method is potentially long-running and blocks until it is done. This is the default for all method, which are unmarked.
  • @valueObject - The objects of this class are immutable. All methods are side effect free.
  • @nonReentrant - This method cannot be called twice at the same time.
  • @threadsafe - This method can safely be called twice at the same time.
  • @caching - If possible, this method uses a cache to calculate the return value.
• Threading and Concurrency
  • Try to avoid instantiating the class Thread directly but rather use a ThreadFactory (in particular the NamedThreadFactory so that your threads are named) or even better an Executor.
  • Spend some time learning about the Java Concurrency library java.util.concurrent.

No @author tags

We don't use @author tags in new code because of the following two reasons:

• Such tags do not reliably point to a person that can be asked questions concerning the tagged file. Quite a few people working on Saros are students which usually stay in project for only a short amount of time.
• Such tags do not accurately represent the actual involvement of/work done by a certain developer. More often than not, developers in our project work in files they did not create. If anyone really wants to determine the authorship of certain files, our version control does a much better job by accurately crediting both the developers and reviewers of each commit.

7. Progress & Cancelation 101

Whenever a method is long-running, i.e. there is a chance that it will take longer than 100ms or involves any external factors such as the user or input/output, the software engineer is responsible to provide a way to track progress of the operation and provide to the caller the possibility to cancel the operation.

If the software engineer does not provide such opportunity the user experience might be reduced. For instance in Saros, there used to be no possibility to cancel a file transfer from one user to the other but just to cancel in between the files. This behavior seems okay, but once we started to see archive files of 10 MB and more, which could only be canceled by disconnecting from the Jabber-Server, the undesireability of the behavior becomes pretty clear.

Fortunately enough there is a straight-forward and simple solution, which also improves the general threading behavior of the application: The class to use is called SubMonitor which implements the IProgressMonitor interface.

Now all methods which are long running, need to be changed to take a SubMonitor as a last parameter (this is our convention):

public Result computeSomething(List input, ..., SubMonitor progress){
  //something
}

Inside those methods, first, we need to initialize the ProgressMonitor with the name of the task and the number of steps this task will take (if the number of steps is unknown we set it to a large integer, 10000 is our convention):

progress.beginTask("Computing Something", input.size());

Now whenever we have made some progress towards this task, we can report this to the monitor:

for (Something some : input) {
  ... process input
  progress.worked(1);
}

At the end of the task, we should report, that we are done with the task:

progress.done()

This basic contract of beginTask(), worked(), and done() is sufficient to achieve the basic uses of progress monitoring.

8. Nesting Progress

In many cases the situtation is a little bit more complicated, as the operation that is long-running is making calls to other long-running operations as well. To solve this problem, we need to create child progress monitors, which consume a given number of work steps from their parent:

public void computeInTwoSteps(IProgressMonitor monitor){
    SubMonitor subMonitor = SubMonitor.convert(
                                     monitor, 
                                     "Compute in two steps", 
                                     2
                                   );
    progress.beginTask("Compute in two steps", 2); 
    computeFirstStep(subMonitor.newChild(1)); 
    computeSecondStep(subMonitor.newChild(1)); 
    progress.done();
}

This code will pass two SubMonitors to the two methods, which then are free to use them just as the parent method did:

public void computeFirstStep(SubMonitor progress){
 
  progress.beginTask("Compute the first step", 140);
  ...
  progress.worked(5); // etc.
  ...
  progress.done();

}

9. Reporting information to the user

A progress monitor provides 3 ways to report information from a long running operation to the user

• The amount of steps already worked as given by worked()
• The name of the task, as set using beginTask(String) and setTaskName(String)
• The name of the sub-task, as set using subTask(String)

This information is typically presented to the user as a Dialog with a message being equal to the taskname of the top level progress monitor, a progress bar showing the growing amount of work already done and a label for the current sub-task which switches everytime the sub-task is being set.

Since the taskName is fixed (by default), only the top level task name is shown to the user. The task name of the nested operations are never shown to the user. To report status messages from nested operations, the sub-task needs to be used:

public void computeInTwoSteps(SubMonitor progress){
 
  progress.beginTask("Compute in two steps", 2);
 
  progress.subTask("Two Step Computation: Step 1");
  computeFirstStep(progress.newChild(1));
 
  progress.subTask("Two Step Computation: Step 2");
  computeSecondStep(progress.newChild(1));
 
  progress.done();

}

10. Dealing with operations of unspecified length

To have a progress dialog on operations for which the amount of steps are unknown, the following solution is recommended:

while (!done()) {
  ... do work

  progress.setWorkRemaining(1000);
  progress.worked(1);
}

This code will advance the progress bar 0,1% of the remaining total of the progress monitor and thus logarithmically approach 100% worked. The percentage 0,1% should be adjusted to achieve 50% progress on the expected number of work steps.

11. Cancellation

To achieve cancellation support in an operation, we should check frequently whether the user has requested that we stop our tasks:

for (Something some : input){
  if (progress.isCanceled())
    return;
  ... process input
  progress.worked(1)
}

The easiest way to response to a request for cancellation is to just return as shown above, but in most cases this is undesirable, because the caller will not know whether the operation finished or not. Instead, methods should rather throw a CancellationException so that a caller can recognize that the operation was canceled:

public BigInteger factorial(int n, SubMonitor progress){
 
  progress.beginTask("Computing Factorial of " + n, n);
 
  BigInteger result = BigInteger.ONE;
 
  for (int i = 1; i < n; i++) {
    if (progress.isCanceled())
      throw new CancellationException();
 
    result = result.multiply(BigInteger.valueOf(i));
    progress.worked(1);
  }
 
  progress.done();
  return result;
}

Btw: It is an convention that we try to avoid InterruptedException for this, because it is a checked exception and thus cumbersome for the caller. To maintain this convention, a method MUST specify whether it is cancelable, by providing the demonstrated JavaDoc tag.

12. Software Design Rules

• Distinguish clearly between the common building blocks of applications, namely:
  • Services - Providers of functionality that can be well encapsulated
  • Entities - Mutable classes that represent the business world. Group these into aggregates.
  • Values - Use the Value Object Pattern to ensure that users of the class can rely on the objects to be immutable and all methods to be side effect free. This helps A LOT when using a class.
• Avoid implementing many interfaces in the same class whereever possible (more than one is necessary only in rare cases). Use nested or anonymous classes instead. It makes it much easier to understand and modify your code.

• The Singleton Pattern is a inferior solution to define the architecture of an application compared to using Dependency Injection (see next section). To achieve the lazy semantics often also associated with the Singleton Pattern you should inject a dependency to a lazy initializer of the component that you actually need instead.

Dependency Injection

• Saros uses a dependency injection framework called PicoContainer, because it improves the readability, changeability and modularization of the architecture of a software application.
• Short introduction:
  • The goal of dependency injection is to solve the basic software engineering problem of how a object of a class A (usually a class representing a component) receives the objects of other classes B and C which it needs during the programm execution.
    • A typical example would be that the application core needs access to a preference object to work.
  • The solution that dependency injection provides is to let this problem be solved by a dependency injection container, which has a defined way to resolve such dependencies.
  • This involves three steps:
    1. Telling the dependency container which classes need to be managed by it
       • In PicoContainer:
         • container.addComponent(A.class)
    2. Changing the classes so that the container can identify what constitutes the classes dependencies.
       • See below
    3. Asking the container for object instances with all their dependencies being set.
       • In PicoContainer:
         • container.getComponent(A.class)
  • For the second step there are three possibilities:
    1. Constructor injection - The dependency container will use the constructor with the least amount of parameters to create an object if it needs one. This is the cleanest type of injection, since the dependencies are available to an instance of the class right from the start.
    2. Annotated field injection - The dependency container will look for fields with special annotation tags (in PicoContainer @Inject) and AFTER the constructor has been called will fill those fields with the dependencies. This type of injection, provides the code with the best readability (constructors with many parameters are difficult to read) but is more difficult to manage, because dependencies are not available in the constructor.
    3. Setter injection - Injection is done via setters. This is a possibility we do not use.
  • Recommendation: Use annotated field injection only from GUI classes. If a constructor has got too much input parameters it may be the time to think again about the design, which should avoid the god class pattern.
• Dependency injection and Eclipse
  • Since Eclipse creates many objects itself such as Views and Actions, we cannot use constructor injection with such components.
  • Instead a reinjector needs to be used which can take an existing object and using annotated field injection will pass the missing dependencies.
• Some literature:
  • Martin Fowler - Inversion of Control Containers and the Dependency Injection pattern
  • PicoContainer introduction

Broadcast Listener

To avoid repeated code blocks in an Observable like

class Observable {
 
  List listeners = new ...
  public void addListener(Listener listener){listeners.add(listener)}
  public void removeListener(Listener listener){...}
 
  public void someMethod() {
    ...
    // notify all listeners
    for (Listener l : listeners) {
      l.notify();
    }
  }
 
  public void someMethod2() {
    ...
    // notify all listeners again
    for (Listener l : listeners) {
     l.notify();
    }
  }
 
}

It is recommended to use a helper class BroadcastListener that provides a method to notify all its registered listeners. The BroadcastListener should be a singleton managed by PicoContainer.

public class BroadcastListener implements Listener {
 
  List listeners = new ...
  public void add(Listener listener){listeners.add(listener)}
  public void remove(Listener listener){...}
 
  public void notify() {
    for (Listener l : listeners) {
      l.notify();
    }
  }
 
}

The code for an Observable becomes therfore much simpler. It only needs to know the BroadcastListener and can easily notify all registered listeners at once.

class Observable {
 
  BroadcastListener listener = new BroadcastListener();
 
  public void someMethod(){
    ...
    listener.notify();
  }
 
  public void someMethod2(){
    ...
    listener.notify();
   }
}

13. Common Templates

Registering a listener on a changing instance:

public void setSharedProject(SharedProject newSharedProject) {
 
  if (sharedProject == newSharedProject)
    return;
 
  // Unregister from old project
  if (sharedProject != null) {
    sharedProject.removeListener(this);
  }
 
  sharedProject = newSharedProject;
 
  // Register to new project
  if (sharedProject != null) {
    sharedProject.addListener(this);
  }
}

14. Documentation

• JavaDoc documentation and names should be meaningful and make the programming element understandable with minimum insight into the code. If your comments make the code more difficult to understand, remove them.
• Don't use single line comments (starting with //) for multi line text, use instead
  

  /*
   * one comment that takes up at least
   * two lines
   */

• Comments should describe complex code in shorter text. Comments like "Create HashMap", "Set Value", "Loop here", or "else" should be removed.

Kinds of comments

This section is based on Steve McConnell's Code Complete, Chapter 32.

There are six categories of comments:

1. Repeat of the Code
   • If something is not complex and thus documentation repeats code, you should not document at all. For instance, don't document getters and setters (an exception would be to explain what the variable actually contains that you are getting).
   • A counter-example:
     

     /**
      * return a stack of String,
      * @param text
      * @return Stack
      */
     
     public Stack getFormatJavadocLine(String text) {
     
       StringTokenizer st = new StringTokenizer(text, "\n");
       Stack stack = new Stack();
     
       while (st.hasMoreTokens()) {
         stack.add(st.nextToken());
       }
     
       return stack;
     }

   • The documentation is absolutely useless as it just repeats the signature and even fails to explain the method name. Furthermore the method name is wrong for the task that is actually performed. The important question whether this method returns a stack of the individual lines in text from top to bottom or bottom to top remains unanswered.
2. Explanation of the Code
   • If the code needs to be explained, it is usually better to improve the code instead of adding comments.
3. Marker in the Code
   • Marker comments are notes for the developers that the work isn't done yet. They should not be left in the code. If you have to mark a section in the code, use „TODO“. This way all marker comments will be standardized and it is easier to locate them.

     /*
      * TODO FIX: Our caller should be able to distinguish whether the
      * query failed or it is an IM client which sends back the message
      */

4. Summary of the Code
   • Comments that summarize the code in a few sentences can be valuable especially for readers who haven't written the code. They can scan these comments more quickly than the code.

     /*
      * move all chess pieces to start position
      */

5. Description of the Code's Intent
   • Intent comments explain the purpose of a section of code. In contrast to the summary comments which operate at the level of the solution the intent comment operates at the level of the problem.

     /*
      * initialize a new chess game
      */

6. Information that cannot possibly be expressed by the Code Itself
   • Some information needs to be written in comments since they cannot be expressed in code. This can be copyright notices, version numbers, notes about the code's design, optimization notes and so on.

Acceptable code comments are summary comments (4.), intent comments (5.), and information that cannot be expressed in code (6.). Markers (3.) are acceptable during development but should be removed before release. Try to avoid comments that repeat (1.) or explain the code (2.).

What to comment

Generally you should document the code starting from the highest level of code hierarchy. This means that all packages need a documentation followed by interfaces and classes. All documentation should be in JavaDoc comments in order to automatically generate HTML source code documentation.

• Each package should be documented in a package-info.java file located in the package folder.
  • The package description should help the reader to understand, what the package is about and what its intent is. It should also inform about terms to adhere when using or modifying this package. Important relations to other packages should be described here.

• Each interface should be documented.
  • The comments in the interface should provide a short description of what you can use it for. For all exposed routines you should at a minimum document each parameter and each return value but hide implementation details.

• Each class should be documented.
  • The description of the class should provide a short overview of what this class is about. Design decisions and limitations should be mentioned as well.

• Methods should be documented, if they are complex enough and it will be helpful for other readers to summarize or explain the purpose of these methods.
• Important objects and variables should also be briefly documented.

15. Before Committing: Formatting and cleaning-up

See here.

16. Error Reporting, Exception Handling and Logging

We expect that all source code used in thesis to deal gracefully with errors. The goals to aim for should be:

• Provide information to the user that something went wrong and how s/he can fix the problem (if). A typical example for this is a failed to log-in to a server, because the password is wrong which should lead to a error-dialog box. In the best of all cases this error message will offer the user to correct the problem or lead him to the place to correct it (for instance a preference dialog).
• Provide information to the developer when some operation failed that should not have (unexpected) and where the problem occurred (stack-trace).
• Provide information about the kind of events that happened internally (tracing/logging). It should be possible to disable these.

The first step to achieve this, is to make sure that you notice when things go wrong. Thus, all Runnables passed to Threads or Executors and all methods called from 3rd party software, such as Actions called from Eclipse, or Listeners from the network API need to be made secure as to catch all RuntimeException that might have slipped up.

Use the following method for this (you might want to pass up RuntimeExceptions up the stack as well):

/**
 * Return a new Runnable which runs the given runnable but catches all
 * RuntimeExceptions and logs them to the given logger.
 *
 * Errors are logged and rethrown.
 *
 * This method does NOT actually run the given runnable, but only wraps it.
 */
public static Runnable wrapSafe(final Logger log, final Runnable runnable) {
  return new Runnable() {
    public void run() {
      try {
        runnable.run();
      } catch (RuntimeException e) {
        log.error("Internal Error:", e);
      } catch (Error e) {
        log.error("Internal Fatal Error:", e);

        // Rethrow errors (such as an OutOfMemoryError)
        throw e;
      }
    }
  };
}

When developing in Eclipse the following code-snippets might help:

• Error reporting to the user can be done using an ErrorDialog:

  Display.getDefault().syncExec(new Runnable() {
      public void run() {
          MessageDialog.openError(
              Display.getDefault().getActiveShell(),
              "Dialog Title", "Error message"
          );
      }
  });

• Error reporting for the developer can be done using the ErrorLog:
  

  YourPlugin.getDefault().getLog().log(
    new Status(IStatus.ERROR, "Plug-In ID goes here", IStatus.ERROR, message, e));

• Input from the user needs always to be checked and untainted on input.
  • Error messages need to be human readable.
  • Exceptions need to be handled correctly, i.e.
    • If the program could do something about the exception it should try (for instance repeat an failed operation).
    • Otherwise an unchecked exception should be thrown to prevent =throws=-clauses littering the code.
    • If you can't handle the exception then throw it back to the caller

Anti-example:

public Javadoc updateJavadoc(String filename, String name,
  String newJavadocText, int isType) {
  Javadoc jd = null;
  try {
    ... Try to update Javadoc ...
  } catch (Exception e) { // No, no, no!
    e.printStackTrace();
  }

  System.out.println("The new javadoc-------\n" + jd);
  return jd;
}

How to do it right:

public Javadoc updateJavadoc(String filename, String name, String newJavadocText, int isType) 
  throws IOException {

  Javadoc jd = null;
  try {
    ... Try to update Javadoc ...
  } catch (IOException e){ // Catch the checked exceptions that occur
    // bring the internal logic back to a stable state (if you can)
    throw e; // let the caller handle this exception
  }

  System.out.println("The new javadoc-------\n" + jd);
  return jd;
}

Logging with Log4J

• We use one private static final logger per class. An editor template for Eclipse:
  private static final Logger LOG = LOgger.getLogger(${enclosing_type}.class);
• We use the following Log-Levels:
  • ERROR An error should be printed if something occurred which is the fault of the developer and which will cause unexpected behavior for the user.
  • WARN A warning should be printed if something occurred which is the fault of the developer but which while not expected to occur should be handled gracefully by the application.
  • INFO Any information that is of interest to the user may be printed using INFO.
  • DEBUG Any information which might be useful for the developer when figuring out what the application did may be printed as DEBUG. The amount of information printed should not cause the performance of the application to suffer (use TRACE for this).
  • TRACE Detailed information about the program execution should use the TRACE level. This information usually is so detailed that the application runs slower and thus should never be enabled in a production version.

Dealing with InterruptedException

• When calling a blocking method, Java uses InterruptedException to signal that the waiting thread was told to stop waiting.
• As a a caller to a blocking method it is your responsibility to deal with the possibility of being interrupted. This is why exception is checked in Java.
• The contract of InterruptedException is the following:
  • If interrupted a method honoring the contract MUST either throw the InterruptedException or set the interrupt-flag.
• Since the InterruptException-contract is assumed to be honored by all methods in Java, there are three ways of dealing with the InterruptedException:
  1. Rethrowing the InterruptedException to tell callers that this method might be interrupted: As we do not like checked exception this is an inferior solution to the problem.
  2. Resetting the interrupt flag
     • It is your responsibility to always reset the Interrupt flag in case you catch the Exception, because somebody who called you might depend on it. This will look like this and is the normal case for 90% of all cases:

       try {
         Thread.sleep(500);
       } catch(InterruptedException e){
         // The line of code will continue after the catch
         Thread.currentThread().interrupt();
       }

     • This is recommended even if you are sure that you will never be interrupted (because the program might change in the future)
     • Special case: Spinning / Busy Waiting
       • If you use Thread.sleep() inside a while() loop, then you cannot use the above pattern without leaving the while loop, because Thread.sleep() (all methods that honor the contract of InterruptedException) will return immediately without sleeping. Thus your while loop becomes a busy waiting loop.
       • If you really do not want to be interruptible, then you need to do the following:
         

         boolean interrupted = false;
         try {
           while (looping){
             // do stuff
             try {
               Thread.sleep(500);
             } catch(InterruptedException e){
               interrupted = true;
             }
           }
         } finally {
           if (interrupted)
             // The line of code will continue after the catch
             Thread.currentThread().interrupt();
         }

  3. Tell others that you violate the contract: Add to the signature of your method, that you do not support the contract of Thread.interrupt(). Be aware that by violating the InterruptedException-contract you put all your callers into violation as well (since if handle an InterruptedException incorrectly they cannot honor the contract anymore). Use the following to tell callers that you do not honor the contract:
     

     /**
      * @nonInterruptable This method does not support being interrupted
      */

• BTW: There is no obligation in the contract for you to exit as quickly as possible.
• For more read: http://www-128.ibm.com/developerworks/java/library/j-jtp05236.html

17. Little Things and Best Practices

• Don't use they keyword "static" at all unless there is very good reason to do so which is backed up by a detailed explanation providing a formal analysis of correctness and perhaps side effects.
• No direct calls of GUI components from the business logic. Keep model and view separated.
• Mark overridden methods with @Override to increase the transparency of the code.
• To convert a primitive to a String use String.valueOf(...). Don't use "" + i.
• To convert from a primitive use Integer.parseInt(...) and similiar.
• Don't use StringBuffer unless you need thread safety, use StringBuilder instead.
• Similarly don't use Vector, use ArrayList
• Use IO Utils when dealing with reading and writing files. Do not try to invent the wheel and read individual bytes out of streams.
  • Do use IOUtils.closeQuitely() for closing streams, as it handles all cases (null, exceptions) in one line
• Never subclass Thread, always pass a Runnable into the constructor instead
• Run your program using -ea and use asserts to let people understand what you were assuming in your code.
• To convert from an IPath to a String use IPath.toPortableString() to convert from a String to an IPath use Path.fromPortableString() (one could also use toString() and new Path(String), but we prefer the from/toPortable version).
• If equals() is implemented in a class it is MANDATORY to implement hashCode() as well, otherwise HashSets and other classes which rely on the contract of hashCode() will not work. To do this correctly always use the Eclipse Generator (Source -> Generate hashCode() and equals()).
• Always document the reason if x.compareTo(y) == 0, but x.equals(y) returns false
• Use enum instead of a bunch of static final int fields. Then you have type safety for related constants and more readable debugging output is possible because you get meaningful names instead of "magic" numbers.
• The creation of new utils classes, which tend to be used by only one other class, is strongly discouraged. Consider the use of inner classes or moving the functionality to existing utils instead.