IMPORTANT NOTE: This document has not yet passed the implementation phase. You should regularily check for updates.
How to write code
This document contains requirements and guidelines for writing good code. Here you will find information about code and javadoc conventions we follow and some good practices. Rules for reviewing the implementation phase will be provided as well.
Code conventions
This section says what you have to comply with when writing code. You should always remember the main guideline of this project: Work towards the goal! Besides what is described in this document, you should take a look at the official coding conventions for the Java programming language from Sun: http://java.sun.com/docs/codeconv/. More links to conventions can be found in the USEFUL_LINKS page.
General rules
You should follow the following general rules when writing your code. The code written should:
- cause 0 errors
- cause 0 warnings
- be easily extensible, maintainable and testable
- be easily readable (applies especially for classes - the need good layout)
- see http://java.sun.com/docs/codeconv/html/CodeConventions.doc2.html#1852 for details on the layout
- be well documented
- use extensively the property library - it is fundamental in our project
- only public static final fields and properties are allowed
- not use the @SuppressWarnings annotation except in the following cases (with caution!):
- unused, when something is used only by reflection
- synthetic-access, when you want to touch something internal with an inner class (although in many cases this may be avoided)
- unchecked, when you are doing something complex with generics (note that this is also avoidable in many cases)
- not add inacurate or unclear XXX and TODO tags
- not break other code (ensured by their tests)
- not contain very long classes/methods
- not contain magin numbers/strings
- not be duplicated
- correctly use logging
- have adequate error handling
- apply to the conventions below
Do not look for quick fixes and workarounds that solve the problem but make the code much more complicated. Instead, look for something simple and elegant that will make changing/adding code at a later stage easier. Bad code leads to bad velocity for the whole project.
Javadoc conventions
There should be no fake JavaDocs (JavaDocs without useful information). Do not repeat the method name in a sentence. Instead, try to explain something that is not easily noticed. "Constructor" is not a good javadoc for an object constructor. The Javadoc should be complete enough and describe clearly each element. See the JAVADOC_CONVENTIONS page for a more detailed description of the conventions.
Good practices
Code reuse
You should not duplicate code - do not have the same functionality on more than one place. This increases the chances of mistakes and makes the refactoring difficult.
In the class BookView we had the following two methods:
@Override
public void pageAdded(Page page) {
PageView pageView = new PageView(this, page);
this.pageViews.add(pageView);
page.getObservable().addObserver(pageView);
this.observable.getInvoker().pageViewCreated(pageView);
}
// some other code
...
private void createChildView(Page page){
PageView p = new PageView(this, page);
this.pageViews.add(p);
this.observable.getInvoker().pageViewCreated(p);
}
As we see, we have code reuse and the private method does not register the view as an observer of the page. This shall become:
public void pageAdded(Page page) {
createChildView(page);
}
// some other code
...
private void createChildView(Page page){
PageView p = new PageView(this, page);
this.pageViews.add(p);
page.getObservable().addObserver(p);
this.observable.getInvoker().pageViewCreated(p);
}
If we do not use the private helper method createChildView in other places, it is redundant and its code should be moved in the pageAdded method.
Properties usage
Do not use any fields except with public static final modifiers. Instead, use properties. The property library offers great advantages and you should learn how to use it. For a tutorial on the properties library, see PRO_LIB_CORE_TUTORIAL.
Final modifier
Usage of the final modifier for classes, methods and especially class members is a good practise and prevents from a number of mistakes. However in most cases you do not need to add a final modifier to method variables and arguments. Here are some examples about the usage of the final modifier.
Classes and methods
When writing a class, always think whether it is supposed to be inherited or no. If it is not, declare it final (that's what most cases are). Otherwise someone may decide to inherit it, which will cause a lot of problems. There are some very famous examples of such mistakes being made: for example java.sql.Date inherited java.util.Date and caused a real mess that you can read about here: http://www.thunderguy.com/semicolon/2003/08/14/java-sql-date-is-not-a-real-date/. The problem all starts from the fact that the documentation of java.util.Date says the class should not be inherited although it is not declared final.
Everything said for classes, applies for methods as well - if a method is not supposed to be overriden, declare it final.
Class members
Class members should be either public static final fields or properties. If a property should not change, make it a FinalProperty.
Method variables and arguments
In most cases it is not necessary to declare method variables and arguments final. However, if you think not doing so may lead to serious mistakes at a later stage, you may do so.
Code smells
Code smells are common mistakes or misusages that are bad practices. Here are some examples of code smells with explanations. More can be found in the CODE_SMELLS page.
Switch statements
The use of switch statements is usually considered code smell. You should think of replacing the switch statement with polymorphism. This doesn't mean that switch statements are always a code smell but it is best if they can be avoided.
Exception swallowing
You should NEVER swallow an exception. Consider the following code:
public void internalFrameActivated(InternalFrameEvent e) {
// show title bar
FrameView.this.observable.getInvoker().setFocus(true);
try {
getParentView().getParentView().swingRepresentation().setSelected(true);
} catch (PropertyVetoException e1) {
// TODO - fix exception handling
e1.printStackTrace();
}
}
Printing the stack trace is NOT exception handling, though it may be useful for debugging. The fact that the method continues no matter whether there is an exception or not in fact means that we are not interested in the result of the method in the 'try' clause. In other words, this means that we are not concerned whether this works or not. Then the example above could be rewritten as follows:
public void internalFrameActivated(InternalFrameEvent e) {
FrameView.this.observable.getInvoker().setFocus(true);
}
If you don't know how to handle an exception, throw it instead of swallowing it. Since PropertyVetoException is a checked exception, you may wrap it in a RuntimeException. When not sure, ask someone from the team for help.
Proper exception handling, without changing the interface is:
public void internalFrameActivated(InternalFrameEvent e) {
// show title bar
FrameView.this.observable.getInvoker().setFocus(true);
try {
getParentView().getParentView().swingRepresentation().setSelected(true);
} catch (PropertyVetoException e1) {
throw new RuntimeException(e1);
}
}
Warning levels
We should use the default warning levels of Eclipse. You can find information on how to change them, go to the COMPILER_SETTINGS page. No warnings are allowed in the code so it is important that your Eclipse settings do not ignore some of them.
Naming Conventions
You should avoid non-descriptive and inaccurate names. A name should be clear enough to suggest what the thing is (given the local context). For example:
private static final int FRAME_CORRECTION_POSITION = 20;
is not clear in the context of the class Frame. It may be more clear in the context of the frameDropped() method but here it is not. A better name would be:
private static final int FRAME_DROP_OFFSET = 20;
For more information on the naming conventions, see the corresponding part of the Java programming language conventions: http://java.sun.com/docs/codeconv/html/CodeConventions.doc8.html#367
Reviewing code
Implementation section requirements
The implementation section for coding tasks should contain:
- a link to the changeset of the commit(s) where the modifications were done
- explanation of the changes made (improvements, added functionality, etc.)
- links to any new classes/packages/modules created
- links to any new auto tests added
Scoring
Reviewers should either follow the standards in this document or comment them in the Comments section of this page. If you state a task does not comply with the standards, point to the requirements that are not met. Scores are in the range 1-5. Here are guidelines for scoring the implementation of coding tasks..
- Score 1 - the implementation does not comply to the standards and conventions in the current document or does not fullfil a task requirement for no reason.
- Score 2 - the implementation fulfills the task requirements but conventions are not followed.
- Score 3 - the implementation complies with the standards and follows the conventions to a certain extent but there are a lot of thing to be improved.
- Score 4 - the implementation complies with the standards and follows the conventions.
- Score 5 - the implementation complies with the standards, follows the conventions and is done in a really good way - code is elegant and there is nothing more to be added - even a person that is not deep into the project can understand it.
All reviews should be motivated. A detailed comment about why an implementation fails is required. For a score of 3 a list of things that have to be improved/added should be provided. Comments are encouraged for higher scores as well. Non-integer scores are STRONGLY disencouraged. If you give a task a score of 3.5, then you probably have not reviewed it thoroughly enough and cannot clearly state whether it is good or not. Once the implementation has been reviewed, it cannot be altered. If you think it is wrong, you should request a super review. Currently all super reviews should be discussed with Milo. Make sure you are able to provide clear arguments of what is wrong before you request a super review.
Comments
- I think that other wiki pages from the important docs connected with coding should be reviewed too. All these pages should have proper links between them. --pap@2009-01-15
- The following line needs to appear in each Sophie file:
Copyright 2008 The University of Southern California. Licensed under the Educational Community License, Version 2.0.
--deyan@2009-03-23
