Error: Macro BackLinksMenu(None) failed

compressed data is corrupt

IMPORTANT NOTE: This page is out of date and will no longer be updated. Its content has been moved to PLATFORM_STANDARDS_CODE

Both implementor and reviewer must be very careful. Please check that you understand all of the above.

For each code task (no matter whether it is a library, external feature, bug fix...), should pass the following check list:

• code
  • The code style is correct to our convention.
  • The code is easily readable.
  • There are no fake JavaDocs (JavaDocs without useful information).
  • The java-doc is complete enough.
    • For each element, the JavaDoc should describe clearly what is it (if this is not easy, then probably the design is bad)
  • It causes 0 errors.
  • It causes 0 warnings.
  • The @SuppressWarnings annotation is used only if really needed.
    • I know 3 cases for it -
      • unused, when something is used only by reflection
      • synthetic-access, when you want to touch something internal with inner class (although in many cases this may be avoided)
      • unchecked, when you are doing something complex with generic (note that this is also avoidable in many cases)
  • No inaccurate, or unclear XXX and TODO tags
  • It should not break other code (ensured by their tests).
  • other suggestions?

• design
  • It is documented
    • (not only in the code).
  • It is simple and understandable.
  • It is easy to use.
    • (using it requires less work / small code)
  • It is error proof (hard to use it in a wrong way)
    • The compiler prevents most errors.
      • (it is better to have an abstract getKind() instead of forcing implementors to call setKind at construction)
      • (final may also help)
    • Most of other errors cause an exception
      • (defensive programming helps here)
  • When a library is used, it is used correctly.
  • There are no representation exposures.
  • There are no GOD (very long) methods.
  • There are no GOD (very long) classes.
  • The coupling is low.
  • No magic numbers (and strings)
  • No array members.
    • (unless there is really a reason)
  • No public instance fields.
  • No public non final fields
  • It has high cohesion.
  • (see ​http://en.wikipedia.org/wiki/Cohesion_(computer_science) )
  • It has low coupling.
    • (see ​http://en.wikipedia.org/wiki/Coupling_%28computer_science%29 )
  • It is possible to state in one sentence what every class or member is/does.
  • Classes and members are named correctly.
  • Every class (or attribute) is self responsible and self complete.
  • No code duplication (copy paste).
  • It is easily testable.
    • (when not, testing helpers are provided to make it such).
  • No other kind of smells.
• It has good package structure.
• It is logging correctly.
• It has adequate error handling.
• No premature optimization.
• No premature abstraction.

• tests
  • It has automatic use case tests, before implementing.
    • (they are very good to demonstrate the design for a library)
  • If it has demos, the demos are actually tests with main method.
    • (demos should be done at design time!)
  • It has enough unit tests to ensure its correctness
    • (listed at design time, passing at implementation time)
  • It has enough integration tests
    • (listed at design time, passing at implementation time)
  • It has enough system tests
    • (listed at design time, passing at implementation time)
  • It has written scenarios for manual testing
    • (when it is an external feature or a bug)
  • The tests had code coverage
    • (good code coverage does not mean good tests, but bad code coverage means bad tests)

• other:
  • Enough information should be provided in the backlog.

Code Reuse

Do not have the same functionality on more than one place - this increases the chances of mistakes and makes it difficult for refactor.

In the class BookView we had the following two methods: As we see we have code reuse, and the private method do not register the view as observer of the page.

        @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);
	}
  
...

	private void createChildView(Page page){
		PageView p = new PageView(this, page);
		this.pageViews.add(p);
		this.observable.getInvoker().pageViewCreated(p);
	}

Shall be:

        private void createChildView(Page page){
		final PageView p = new PageView(this, page);
		this.pageViews.add(p);
		this.observable.getInvoker().pageViewCreated(p);
		page.getObservable().addObserver(p);
	}

...
        public void pageAdded(Page page) {		
		createChildView(page);
	}

Switch statements

Very often the use of switch statements can be considered code smell. We should think of replacing the switch statement with polymorphism. Thus, this doesn't mean that switch statements are always code smell,but we should think whether we can avoid them before writing them.

Exception swallowing

Extremely large number of improper exception handlings appear in the code. NEVER swallow an exception!!!

         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 debug. The fact that the method continues running the same way both if there is an exception and if there isn't in fact means that we are not interested in the result of the method in the 'try' clause. If we write the exception handling like this there is no matter if it works ok, or not - it is all the same for us. Indeed the example below is the same as if we write:

         public void internalFrameActivated(InternalFrameEvent e) {
		FrameView.this.observable.getInvoker().setFocus(true);	
	}

Always rethrow the exception if You dont have information how to handle it. Since PropertyVetoException is checked exception, it may be good to wrap it in a RuntimeException. Consult other people and think of exception handling policy.

Proper exception handling, without change of 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);
		}
	}

Global variables

Do NOT use global variables as a way to pass arguments between non-public methods. This is an unclear method, potentially very dangerous - a lot of mistakes can be done using it.

Use final

Usage of final modifier for classes, methods,and especially class members and method variables is a very good practise and prevents from a number of mistakes.

classes

When writing a class, one of the things we have to think of is - 'is our class designed in a way that it can be inherited?' In 90% of the cases the answer of this question is 'NO'. However if we don't make it final, some people may decide to inherit it and this will cause a lot of problems. There are some very famous examples of such mistakes being made: java.sql.Date inherits java.util.Date and it causes a lot of problems - You can read about them 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 in the documentation of java.util.Date it is written that the class should not be inherited,but it was not declared final. Always put final modifier for your classes if they are not designed to be inherited.

class methods

Sometimes we design our classes for inheritance but there are some public methods that are critical and we do not want the inheritant to override them - again put final modifier. In our module structure we should add a get instance functionality when creating new module class. Here is the template: trunk/sophie2-platform/dev-tools/eclipse_templates/module_templates/module_instance.xml

class variables

When we have a class member that is initialized in the constructor and should never be changed through all the instance life - ALWAYS put final modifier. This will prevent someone change the refference of that class member, that would have lead to wrong behaviour.

In FrameView class, we have PageView parent, that was has to be final but it is not.

private PageView parent;

Replaced with:

private final PageView parent;

method variables

The same as class members.

        public void saveAs() {
		File file = selectSaveFile();
		if(file != null){
			this.book.saveTo(file);
		}
	}

file shall be declared private.

Name Conventions Common Mistakes

• Avoid non descriptive 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;

• Avoid inaccurate names

Comments

• A better structure is needed - now there are headings of all sizes and even a bullet list in the begining. Some of this may go in CODE_TASKS_REQUIREMENTS, see CODE_TASKS_REQUIREMENTS_R1?. --boyan@2009-01-12
• Properties usage should also be stated here. This makes most of the things aboute filelds useless. --pap@2009-01-15
• The section about the 'final' modifier is not good enough. Actually you shouldn't put a final keyword whenever you declare a variable. Final method variables/arguments are usually useless and should be removed. --pap@2009-01-15
• Manual tests are not a coding requirement. --pap@2009-01-15
• The easy testability should be paid attention to. --pap@2009-01-15
• There are some links to external code conventions in the [USEFUL_LINKS] page. Perhaps some of the things written there or at least the links should be put in this document. --pap@2009-01-15
• Good class layout should be put as a requirement. --pap@2009-01-15
• Shouldn't the page be renamed to PLATFORM_STANDARDS_CODE ? --pap@2009-01-15
• 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
• You could replace the BackLinksMenu macro with a TOC one. it may look better. --pap@2009-01-15