Changes between Initial Version and Version 1 of PLATFORM_STANDARDS_CODE


Ignore:
Timestamp:
01/27/09 12:54:54 (16 years ago)
Author:
boyan
Comment:

--

Legend:

Unmodified
Added
Removed
Modified

  • PLATFORM_STANDARDS_CODE

    v1 v1  
     1[[BackLinksMenu]] 
     2[[PageOutline]] 
     3 
     4'''IMPORTANT NOTE:''' This document has not yet passed the implementation phase. You should regularily check for updates. 
     5 
     6= How to write code = 
     7This 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. 
     8 
     9== Code conventions == 
     10This 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 [wiki:USEFUL_LINKS] page. 
     11 
     12=== General rules === 
     13You should follow the following general rules when writing your code. The code written should: 
     14 * '''cause 0 errors''' 
     15 * '''cause 0 warnings''' 
     16 * be easily extensible, maintainable and testable 
     17 * be easily readable (applies especially for classes - the need good layout) 
     18  * see http://java.sun.com/docs/codeconv/html/CodeConventions.doc2.html#1852 for details on the layout 
     19 * be well documented 
     20 * use extensively the property library - it is fundamental in our project 
     21  * only public static final fields and properties are allowed 
     22 * not use the @SuppressWarnings annotation except in the following cases (with caution!): 
     23  * unused, when something is used only by reflection 
     24  * synthetic-access, when you want to touch something internal with an inner class (although in many cases this may be avoided) 
     25  * unchecked, when you are doing something complex with generics (note that this is also avoidable in many cases) 
     26 * not add inacurate or unclear XXX and TODO tags 
     27 * not break other code (ensured by their tests) 
     28 * not contain very long classes/methods 
     29 * not contain magin numbers/strings 
     30 * not be duplicated 
     31 * correctly use logging 
     32 * have adequate error handling 
     33 * apply to the conventions below 
     34 
     35Do 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. 
     36 
     37=== Javadoc conventions === 
     38There 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 [wiki:JAVADOC_CONVENTIONS] page for a more detailed description of the conventions. 
     39 
     40=== Good practices === 
     41 
     42==== Code reuse ==== 
     43You 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. 
     44 
     45In the class BookView we had the following two methods: 
     46{{{ 
     47        @Override 
     48        public void pageAdded(Page page) {               
     49                PageView pageView = new PageView(this, page); 
     50                this.pageViews.add(pageView); 
     51                page.getObservable().addObserver(pageView); 
     52                this.observable.getInvoker().pageViewCreated(pageView); 
     53        }   
     54        // some other code 
     55        ... 
     56        private void createChildView(Page page){ 
     57                PageView p = new PageView(this, page); 
     58                this.pageViews.add(p); 
     59                this.observable.getInvoker().pageViewCreated(p); 
     60        } 
     61}}} 
     62 
     63As we see, we have code reuse and the private method does not register the view as an observer of the page. This shall become: 
     64{{{ 
     65        public void pageAdded(Page page) {               
     66                createChildView(page); 
     67        } 
     68        // some other code 
     69        ... 
     70        private void createChildView(Page page){ 
     71                PageView p = new PageView(this, page); 
     72                this.pageViews.add(p); 
     73                page.getObservable().addObserver(p); 
     74                this.observable.getInvoker().pageViewCreated(p);                 
     75        } 
     76}}} 
     77 
     78If 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. 
     79 
     80==== Properties usage ==== 
     81Do 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 [wiki:PRO_LIB_CORE_TUTORIAL]. 
     82 
     83==== Final modifier ==== 
     84Usage 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. 
     85 
     86===== Classes and methods ===== 
     87When 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. 
     88 
     89Everything said for classes, applies for methods as well - if a method is not supposed to be overriden, declare it final. 
     90 
     91===== Class members ===== 
     92Class members should be either public static final fields or properties. If a property should not change, make it a FinalProperty. 
     93 
     94===== Method variables and arguments ===== 
     95In 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. 
     96 
     97=== Code smells === 
     98Code 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 [wiki:CODE_SMELLS] page. 
     99 
     100==== Switch statements ==== 
     101 
     102The 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. 
     103 
     104==== Exception swallowing ==== 
     105You should NEVER swallow an exception. Consider the following code: 
     106{{{ 
     107         public void internalFrameActivated(InternalFrameEvent e) { 
     108                // show title bar 
     109                FrameView.this.observable.getInvoker().setFocus(true);           
     110                try { 
     111                        getParentView().getParentView().swingRepresentation().setSelected(true); 
     112                } catch (PropertyVetoException e1) { 
     113                        // TODO - fix exception handling 
     114                        e1.printStackTrace(); 
     115                } 
     116        } 
     117}}} 
     118 
     119Printing 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: 
     120{{{ 
     121         public void internalFrameActivated(InternalFrameEvent e) { 
     122                FrameView.this.observable.getInvoker().setFocus(true);   
     123        } 
     124}}} 
     125 
     126If 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. 
     127 
     128Proper exception handling, without changing the interface is: 
     129{{{ 
     130        public void internalFrameActivated(InternalFrameEvent e) { 
     131                // show title bar 
     132                FrameView.this.observable.getInvoker().setFocus(true); 
     133                try { 
     134                        getParentView().getParentView().swingRepresentation().setSelected(true); 
     135                } catch (PropertyVetoException e1) { 
     136                        throw new RuntimeException(e1); 
     137                } 
     138        } 
     139}}} 
     140 
     141=== Warning levels === 
     142We should use the default warning levels of Eclipse. You can find information on how to change them, go to the [wiki:COMPILER_SETTINGS] page. No warnings are allowed in the code so it is important that your Eclipse settings do not ignore some of them. 
     143 
     144=== Naming Conventions === 
     145You 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: 
     146{{{ 
     147private static final int FRAME_CORRECTION_POSITION = 20; 
     148}}} 
     149is 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: 
     150{{{ 
     151private static final int FRAME_DROP_OFFSET = 20; 
     152}}} 
     153For more information on the naming conventions, see the corresponding part of the Java programming language conventions: 
     154http://java.sun.com/docs/codeconv/html/CodeConventions.doc8.html#367 
     155 
     156== Reviewing code == 
     157 
     158=== Implementation section requirements === 
     159The implementation section for coding tasks should contain: 
     160 * a link to the changeset of the commit(s) where the modifications were done 
     161 * explanation of the changes made (improvements, added functionality, etc.) 
     162 * links to any new classes/packages/modules created 
     163 * links to any new auto tests added 
     164 
     165=== Scoring === 
     166 
     167Reviewers 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.. 
     168    * 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. 
     169    * Score 2 - the implementation fulfills the task requirements but conventions are not followed. 
     170    * 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. 
     171    * Score 4 - the implementation complies with the standards and follows the conventions. 
     172    * 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.  
     173 
     174All 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.  
     175 
     176= Comments = 
     177 * 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