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