ADempiere Best Practices

From ADempiere
Revision as of 11:31, 19 September 2011 by Kittiu (Talk) (wikify)

Jump to: navigation, search
This Wiki is read-only for reference purposes to avoid broken links.

Contents

Overview

This page outlines the practices to be followed for development within Adempiere. The rules and processes described are intended to ensure that the project is able to maintain the high standard of quality that is expected from a business critical application.

Team

  1. Teo Sarca [1]
  2. Cristina Ghita Arhipac : Libero stabilization
  3. Trifon D3Soft
  4. Victor Perez e-Evolution
  5. Dominik Zajac BayCIX GmbH
  6. Carlos Ruiz - just read links below\
  7. Paul Bowden
  8. Redhuan D. Oon

Trunk Commit Policy

  1. The following guidelines describe the best practice when committing code in the Adempiere repository.

    General principles

  2. The ability to commit to the Adempiere SVN is a privilege, not a right.
  3. To have autonomy you need a high degree of responsibility.
    1. Committers are expected to take responsibility for their actions.
    2. If you break something, you should make every effort to fix it yourself, rather than relying on others to clean up for you. [2]

    Documentation

  4. All changes to the code base need to be adequately documented so that others can easily find out why a change was made and have the necessary information to allow them to assist with maintaining the new code.
    1. The level of documentation should be in proportion to the degree the proposed change will impact on the behaviour of Adempiere.
  5. All commits must have an associated Bug Report or Feature Request.
    1. The absolute minimum requirement for documentation is that a SourceForge tracker is opened and referenced in the commit.
    2. Trackers must be descriptive. One-line trackers are not sufficient to allow others to complete a meaningful review of your work.
    3. If you're fixing a bug you should provide steps to reproduce the bug in GardenWorld or System.
    4. If it's not easily reproducible in GardenWorld, explain in detail what the issue is, in which scenarios the issue appears and the proposed solution (if any).
    5. If you are reporting a feature request also provide a suggested use case. When you have implemented the feature request you must then explain how to use the new functionality.
    6. Larger changes are better documented in a a wiki page that also provides end user guidance.
    7. It is not acceptable to leave it up to others to "read the code" to work out how your contribution works. [3]
  6. Technical Documentation. Ideally the committer should endeavour to provide some technical documentation. This ensures that if the original committer is unavailable, others will be able to maintain the code without resorting to reverse engineering.

    Approval

  7. Bug fixing can be done directly. If you are confident that you have found a bug and that you have a fix that does not have any adverse side effects, it is acceptable to commit directly without waiting for community consultation.
    1. However, if there is any doubt, committers are advised to request further feedback before proceeding. [4]
  8. Feature requests require community approval. To ensure that all new features will be useful to the community at large and properly implemented a process of community consultation must be undertaken.
    1. This involves proposing the contribution, addressing any concerns raised by community members (paying particular attention to functional specialists), and receiving a positive vote for the request. See the section on voting below.
    2. Getting community approval for implementing a feature request is not sufficient to guarantee acceptance of a contribution. All the other requirements outlined here must also be met.
  9. New features entering trunk should be submitted for review in an separate branch or through a patch
    1. Community approval may be withheld until others are able to review the actual code, either by the code being committed to a "sandbox" branch, or submitted as a patch.
    2. Other committers may vote against a proposed feature until such a submission is provided for them, if they do so, they should make every effort to review the submission as quickly as possible.[5]

    Implementation

  10. The implementation in code of a bug fix or feature contribution must adhere to the following guidelines.
    1. To integrate into trunk a contribution must be complete, documented and testable
    2. Do not commit half finished code into trunk in the hope that someone else will take it over.
    3. If you wish to contribute work that you are unable to complete, supply it as a patch or separate branch. [6]
  11. Don't drop existing elements or functions.
    1. Don't drop "not-added-by-you" things (db elements or functionalities). We must assume that any existing part of Adempiere may be being used by someone's implementation.
    2. Unless there is a good reason to drop it, for example functionality is really broken, do not drop it.
    3. If there is a valid reason then please first ask in forums before considering the drop. [7]
  12. Always review collateral effects of your changes
    1. Every time you touch one line of code or application dictionary please review the dependencies (collaterals). Eclipse helps a lot pointing to the name of the method/variable + right click + references -> workspace. For other code (like callouts, processes, etc) sometimes you need to check the dictionary to see where is it called (I normally review Adempiere_pg.dmp).[8]
  13. Do not commit a change if you only implemented it for one database
    1. We officially support multiple databases and all changes should work exactly the same on both every database platform. Do not assume that someone else will port it for you. [9] and [10]
  14. Support both Java 5 and Java 6
    1. We officially support both these versions of Java. If you are coding against Java 6 please ensure that code is backwards compatible with 5. [11]
  15. Only use the current trunk version of ModelGenerator
    1. There must be just one official version of ModelGenerator. Private versions of ModelGenerator must be avoided. [12]

    Code Style

  16. Code must be properly indented
    1. But don't re-indent fully working code just because it doesn't look exactly right in your IDE.
      The code might be indented nicely in someone elses IDE and if we keep reformatting according to our particular IDE we'll end up with a lot of commits but nothing really done. We won't be able to track "real" changes. [13]
  17. Proper usage of variable names [14]
  18. All comments and variables in english [15]
  19. No duplicated code [16]
  20. Use constants or MSysConfig instead of hardcoded things [17]

    Committing

  21. Follow the best practices for using SVN when committing. [18]
  22. All commits should be atomic:
    1. that is they are the complete code patch that addresses a Bug Report [BR] or Feature Request [FR] in sourceforge.
    2. If the contribution is unusually large it may be committed in small parts but then commit should be distinct functional parts.
    3. Preferably a commit must solve one and only one tracker - this is to ease integration with other branches or maintained versions.
  23. Commit Early in YOUR day:
    1. Only commit if you are around to support or even revert in case of a problems.
  24. Update before commit:
    1. Do an SVN update and ensure all still compiles locally before you commit.
  25. Reference the BR/FR in the commit comments:
    1. include the full url to the sourceforge BR/FR, but also include a short, but descriptive, comment that does not require the reader to go to the BR/FR in sourceforge unless they require details. Example:
      [ 2354040 ] Implementation Replication Mode, Type, Event
      http://sourceforge.net/tracker/index.php?func=detail&aid=2354040&group_id=176962&atid=879335
      Enhance replication to allow export and import of documents and then execute the document status action.
  26. Synchronise & Build:
    1. after committing sync again with the repository and confirm all still builds without errors.
  27. On successful commit Update the BR/FR:
    1. include the SVN revision reference and set the status & resolution fields as appropriate.

    Rules suggested on Nov-2007 to reach TRUNK

    http://sourceforge.net/forum/forum.php?thread_id=1881039&forum_id=611167

  28. Proof of stability. This should be base on community feedback on binary release. Optional: Production site, unit and functional test case.
  29. Sample Data. There should be some sample data available.
  30. Maintainer. Must have dedicated maintainer ( individual or company ). Must follow existing Adempiere Developer's criteria.
  31. Branding and copyright issue. No vendor branding, must prove to be original work.
  32. Migration script. Support all supported databases, i.e oracle and postgresql and pl/java
  33. Migration scripts for ADempiere >342 + 353a shouldn't use or implement new pl/java functions.
  34. Migration Process COMPLETE and documented. For new module that replace or enhance existing functionality, it is very important to have the migration process documented.

    Voting

    For new functionality

  35. Vote takes place in SourceForge forums or trackers
    1. Active members of the community are entitled to vote
    2. A minimum of 72 hours must pass from the call for a vote before the matter can be considered decided
    3. The vote is decided in favour of the simple majority of the participating voters. At least one vote most be received for a vote to count.
    4. A positive vote only indicates that the proposed new functionality is in principle suitable for inclusion in Adempiere. It does not override a fact that a particular implementation of that functionality must also meet all the other requirements listed above.

    Suggestions about hiring sponsored developments to ease inclusion in trunk

    http://sourceforge.net/forum/forum.php?thread_id=1785728&forum_id=611167

  36. See also Sponsorship rules

    Advices suggested on Jul-2007

    License GPLv2

  37. Sponsored development must be released Open Source with the same license from Adempiere (currently GPLv2)
  38. If some development is put in wiki, discussed with community, enhanced with community ideas, etc.
  39. The best practice can be to release it with the same Adempiere license. Very possibly community would not help or opine in a sponsored development that will be released as proprietary.

    Contributor Agreement

  40. All sponsored development must be contributed signing an Adempiere Contributor Agreement (just one agreement by developer) like the one proposed here: Contributor_Agreement
  41. Initially contributor agreements must be sent to a selected trustee.
  42. This is helping to ensure the first clause. It's a common practice in Open Source projects after the SCO vs IBM case.

    Peer review

  43. Sponsored development must follow peer review from a committer to be included in trunk
    1. if the developer is a committer, then must have peer review from other committer (same as all contributions in trunk)

    Tools used

  44. In order to ease inclusion in core, the development must follow guidelines from the project - about tool used for integration:
    1. SQL migration scripts
    2. if the developer have problems with migration scripts, he must at least provide 2pack package

    Must follow Adempiere conventions

  45. Developer must follow Adempiere conventions, i.e. code, directory tree, naming
  46. Adempiere conventions are not clearly written, but Adempiere is big enough to get some conventions just looking what currently we have.

    Test Cases

  47. It would be good developer provide/document simple test cases for the contribution
  48. It's a good advice for sponsors when hiring a developer put the condition that deliverables must include documentation and test cases. The developer must know if the money is enough to make such work.
  49. TestFramework Prototype framework for creating user tests

    Visibility

  50. Try to keep updated the advance in a wiki page
  51. Simple advice that also can be put as a condition from sponsor to developer in order to maximize the community involvement.

    About branches

  52. Branches are allowed (and in fact they're encouraged) to conduct experiments, to work in specific projects or in tasks, or to coordinate team work.
  53. Private branches are allowed - owner(s) of branch rule the branch (all these rules are suggested in this case, but the owner(s) can expand or shrink these rules)

    About releases

  54. Official releases must come from trunk
  55. Official release must be tagged as alpha, beta, release candidate or stable.
  56. This is subject to the number of errors open, the completeness of functionalities, and other issues related to quality of release.
  57. Unofficial releases are allowed from branches
    1. in this case the release must have the name of the branch
    2. in this case the release must NOT be tagged as alpha, beta, release candidate or stable, unless community votes and after a review of open bugs, completeness of functionalities, and other issues related to quality of release.

    Suggested policies for extensions

    Meeting 2008-11-05

  58. According to suggestions received on meeting nov-5-2008 convoked by the more boring ruler.

    Generate ID's from centralized dict

    Assign specific entity type and prefix rules

  59. It's suggested also to follow entity type and prefix rules for all columns and tables created.

    Build on top and not changing kernel

  60. Sometime an extension might replace some code in core but that is usually just a problem in the core rather than changing the definition of an extension
  61. Definition of kernel: common functionalities, like PO, like window management, etc - none related to business rules ...

    Work in a branch

  62. It's encouraged to work in an adempiere branch and open for adempiere committers. But anyways it can be analyzed if extension developer wants to open an own project in sourceforge (or any open source hosting site).
  63. You don't need permissions to experiment in your own branch/project - and add things to core. If you want something integrated in trunk you must follow rules stated in this document.
  64. Extension developers must have a playground where they can develop without asking too much about permissions - following some recommendations to allow easy integration.

    If several extensions are in competence - is suggested to let them compete as extensions, not including any on trunk

    Integrating several extensions at the same time can lead to column collision problems.

    Follow the recommended extension architecture

  65. Write Callouts
  66. Write ModelValidator
  67. Don't generate model for your columns (but you can for your new tables), instead of this use the generic getters and setters available in PO.java

    License

  68. License must be GPLv2.

    No hiding sources

  69. It's encouraged to avoid the SaaS loophole to hide sources (or any other loophole within this license).
  70. Code must be open for everybody in any release.

    Certification

  71. If you follow the suggested policies here - you can be certified as adempiere friendly extension - and we can consider including it within the official release.

    Coding Standards

    Suggestion & Questions Coding Standards

    Coding Style

  72. See Eclipse Code Formatter Profile .
  73. Do not use the conversion from Integer to int, Double to double and so on, because from java5 is doing autoboxing.
  74. Allways use bracelets {}. It is improving code readability and can evict hard to detect bugs.
  75. Don't use transaction names if is not necessary. Better put null.
  76. Known issues:
    1. At present eclipse formatter is not supporting fluent interfaces (see eclipse bug #196001)

    How to change parameters of existing method from ADempiere core

  77. ALWAYS keep the old method for backward compatibility and add the @deprecated tag in the javadoc of the method @deprecated since 3.5.3a. Please use {@link #myMethod(...)} instead
  78. Search for all places from ADempiere core where the old method is using and replace with the new one. In this way you will leave the old method with no calls in core and ready to be dropped after some releases.

    How to use Adempiere Query class?

    How to return only ONE persistent object?

  79. StringBuilder whereClause = new StringBuilder();
    		whereClause.append("AD_Client_ID=?");          // #1
    		whereClause.append(" AND AD_Org_ID=?");        // #2
    		whereClause.append(" AND C_AcctSchema_ID=?");  // #3
    		whereClause.append(" AND Account_ID=?");       // #4
    
     MAccount existingAccount = new Query(ctx, I_C_ValidCombination.Table_Name, whereClause.toString(), null)
     		.setParameters(new Object[]{AD_Client_ID, AD_Org_ID, C_AcctSchema_ID, Account_ID})
     		.first();
    
  80. Use the interface to put the Table Name I_C_ValidCombination.Table_Name
  81. Use the StringBuilder when the Where Clause is built
  82. Use the final String whereClause when the Where Clause is not built If you know that your query should return ONLY one result, then you can assert this, and use firstOnly method instead of first method:
  83.  MAccount existingAccount = new Query(ctx, I_C_ValidCombination.Table_Name, whereClause, null)
     		.setParameters(new Object[]{AD_Client_ID, AD_Org_ID, C_AcctSchema_ID, Account_ID})
     		.firstOnly();
    

    How to return ARRAY of objects?

  84.  public static MAchievement[] getOfMeasure (Properties ctx, int PA_Measure_ID)
     {
     	final String whereClause = COLUMNNAME_PA_Measure_ID+"=?"; 
     	List<MAchievement> list = new Query(ctx, I_PA_Achievement.Table_Name, whereClause, null)
     		.setParameters(new Object[]{PA_Measure_ID})
     		.setOrderBy(COLUMNNAME_SeqNo+", "+COLUMNNAME_DateDoc)
     		.list();
     	return list.toArray(new MAchievement[list.size()]);
     }
    

    How to return ARRAY of objects and process elements?

  85. public static MAchievement[] getOfMeasure (Properties ctx, int PA_Measure_ID)
    {
    	String whereClause = COLUMNNAME_PA_Measure_ID+"=? AND "+COLUMNNAME_IsAchieved+"=?"; 
    	List<MAchievement> list = new Query(ctx, MAchievement.Table_Name, whereClause, null)
    		.setParameters(new Object[]{PA_Measure_ID, true})
    		.setOrderBy(COLUMNNAME_SeqNo+", "+COLUMNNAME_DateDoc)
    		.list();
    	for(MAchievement achievement : list)
    	{
    	  s_log.fine(" - " + achievement);
    	  // do some processing here
    	}
    	return list.toArray(new MAchievement[list.size()]);
    }
    

    How to return one member of an object?

  86. 	public static int getWindow_ID(String windowName)
            {
    	    int retValue = 0;
     	    String whereClause = COLUMNNAME_Name+"=?";
    	    MWindow win = new Query(Env.getCtx(), MWindow.Table_Name, whereClause, null)
    		  .setParameters(new Object[]{windowName})
     		  .first();	
     	     return = win.getAD_Window_ID(); 
            }
     

    How to pass Timestamp parameter?

  87. Timestamp dateAcct = ...;
    String trxName = ...;
    
    StringBuilder whereClause = new StringBuilder();
    		whereClause.append("C_CashBook_ID=?");				//	#1
    		whereClause.append(" AND TRUNC(StatementDate)=?");		//	#2
    		whereClause.append(" AND Processed=?");				//	#3
    		
    MCash retValue = new Query(ctx, MCash.Table_Name, whereClause.toString(), trxName)
    	.setParameters(new Object[]{C_CashBook_ID, TimeUtil.getDay(dateAcct), true})
    	.first()
    ;
    

    How to use Query class with complex where clause: EXISTS?

  88. StringBuilder whereClause = new StringBuilder();
    	whereClause.append("C_Cash.AD_Org_ID=?");			// #1
    	whereClause.append(" AND TRUNC(C_Cash.StatementDate)=?");	// #2
    	whereClause.append(" AND C_Cash.Processed='N'");		
    	whereClause.append(" AND EXISTS (SELECT * FROM C_CashBook cb");
    	whereClause.append(" WHERE C_Cash.C_CashBook_ID=cb.C_CashBook_ID AND cb.AD_Org_ID=C_Cash.AD_Org_ID");
    	whereClause.append(" AND cb.C_Currency_ID=?)");			// #3
    MCash retValue = new Query(ctx, MCash.Table_Name, whereClause, trxName)
    	.setParameters(new Object[]{AD_Org_ID,TimeUtil.getDay(dateAcct),C_Currency_ID})
    	.first()
    ;
    

    How to use Adempiere Callout class?

  89. For new callout code i always recomend to use GridTabWrapper class that is wrapping an GridTab object to an "persistent interface". For more info, there is an example on javadoc of the class. I am copy-paste here too:
  90.  
    I_A_Asset_Disposed bean = GridTabWrapper.create(mTab, I_A_Asset_Disposed.class);  
    Timestamp dateDoc = (Timestamp)value;  
    bean.setDateAcct(dateDoc);  
    bean.setA_Disposed_Date(dateDoc);  
    

    Benefits:

    • cleaner code
    • write algorithms once (and not 1 method for PO and other for GridTab)
    • eliminate error prone strings

    How to use PreparedStatement/ResultSet ?

  91. If you really need to use JDBC queries, then here is a template for that:
     final String sql = "''your SQL SELECT code''";
     PreparedStatement pstmt = null;
     ResultSet rs = null;
     try
     {
          pstmt = DB.prepareStatement(sql, trxName);
          DB.setParameters(pstmt, new Object[]{...''parameters''...});
          rs = pstmt.executeQuery();
          while(rs.next())
          {
              ''// process fetched row''
          }
     }
     // If your method is not throwing Exception or SQLException you need this block to catch SQLException
     // and convert them to unchecked DBException
     catch (SQLException e)
     {
          throw new DBException(e, sql);
     }
     // '''ALWAYS''' close your ResultSet in a finally statement
     finally
     {
          DB.close(rs, pstmt);
          rs = null; pstmt = null;
     }
    

    Always use DB.getSQLValue*Ex

  92. DB.getSQLValue*Ex methods which have same functionality as their counterpart but in case of an SQLException(checked) then a DBException(unchecked) will be thrown.
  93. This will help us to assure data integrity and to distinguish between exceptions and null values. See: FR 2448461 - Introduce DB.getSQLValue*Ex methods

    Always use Trx.run methods

  94. If you want to run a fragment of code that is changing the database data, and if something if failing you need to rollback entire transaction or to rollback to a savepoint, then Trx.run methods are the best option:
  95. rx.run(TrxRunnable r) - creates a new transaction, runs the runnable object and if something fails then rollbacks the transaction and throw AdempiereException (extends RuntimeException). If all is ok, the transaction is commited.
  96. Trx.run(String trxName, TrxRunnable r) - similar with previous run method, but instead of creating a new transaction it is creating a new savepoint.
    Example 1:
        ...
        /**
         * saves the partner, user and employee
         */
        private void cmd_save()
        {
            Trx.run(new TrxRunnable() {
                public void run(String trxName) {
                    MBPartner bp = new MBPartner(getCtx(), bpartner_id, trxName);
                    bp.setValue(fValue.getText());
                    bp.setName(fName.getText());
                    if (bp.get_ID() <= 0) {
                        bp.setIsEmployee(true);
                    }
                    bp.saveEx();
                    ......
                }
            });
        }
    
    Example 2:
        ...
        /**
         * get Connection object
         */
        private void cmd_save()
        {
            Trx.run(new TrxRunnable() {
                public void run(String trxName) {
                    try
                    {
                        Trx trx = Trx.get(trxName, true);
                        conn = trx.getConnection();
                        ...
                    }
                    catch (SQLException e)
                    {
                        throw new AdempiereException(e);
                    }
                }
            });
        }
    

    References

    EE Best Practices

    Precise Java

    Sugesstion & Questions Coding Style

  97. When on a single thread class, StringBuilder should be used for String concatenation. If the class is not single threaded, then, StringBuffer should be used for String concatenation.

    Testing Policy

  98. Submit your changes to local testing or a peer review before committing unless you are approved by 1st level committer
  99. Publish your test results pls edit according to SF link findings above)

    Test Units

  100. The use of FitnesseSlim is thoroughly explored in Cost Engine/Testing with a complete case contribution.

    Documentation Policy

  101. Document your sourcecode changes in this wiki under appropriate topic.
  102. Solicit help from others if you cannot document well. Or just start a stub and allow others to expand it.
  103. Intentionally hiding information may get your contribution categorised as proprietary and not fit for admission into trunk.
  104. New features, windows or fields need help text at least in english.

    Communication

    Bringing ideas (or collecting votes) for a development or bug fix

  105. open a discussion forum and try to keep the discussion in forums (it can be in a tracker, but we have noticed that forums get much more attention than trackers)
  106. after the discussion ends in some proposal then open a tracker - and add a link in the comment to the forum THREAD (to the thread, not to a single message)
  107. after tracker is opened try to keep the discussion on the tracker - unfortunately we cannot close the forum thread (still, maybe in phpbb we can)

    When committing

  108. Try to keep all communication related to one theme in one single thread
  109. Every commit must have a related tracker previously opened
  110. It's recommended the commit message have a link to the tracker and a brief explanation of what was done
  111. After the commit add a comment in tracker pointing to the review, it's recommended to add a link to the svn log
  112. To make comments about code please use the tracker - this is to keep ALL information related in just one site
    1. Please avoid writing to the cvslog maillist - writing comments there is spreading related information, so people trying to follow history of an issue must review forums, then trackers, then maillist. This policy is trying to keep all information related and linked properly.

    Lawful Vote and Review of Best Practices

  113. For more info please see Project_Charter#Political.
  114. Review is now ongoing until 31st December 2008, where it is accepted and voted en bloc by the whole community in January 2009.
  115. It has to be a full turn-out consensus vote first time round from the active common members in order for it to carry the weight of enforceable law.
  116. Subsequent reviews shall be periodic minimally each quarter and not amended adhoc prior to review date.
  117. Next review date should be not earlier than March 2009.

    Mentor Policy

  118. All committers should also seek a mentor or coach among the senior committers already mentioned in the Commit Committee.
  119. Mentees (who are under Mentors) have to assist in providing review and other administrative assistance (this is due to resources constraints at the moment to manage the team).

    Adempiere Official Domains and Subdomains

    Fair use

  120. Some policies were proposed on this thread:
    1. Money collected from these sites (i.e. advertising) must go to German Foundation and help to sustain the project
    2. Site sponsor, site seeder, and site maintainer can be advertised within a sponsors page and with a little box at the end of each page
    3. If there are zkwebui interface then some advertising links are allowed in the initial page
    4. Collecting visitor statistics must be made public for all community, or at least to project admins on request
    5. The sites must not collect user information - ask for registration - or anything about (unless by the nature of the site it's needed)
    6. No forums or support must happen on these sites - all support must be redirected to sourceforge forums
    7. No wiki must be set up on these sites - all documentation must be redirected to adempiere wiki
    8. The maintainer must be active, it the website becomes outdated more than one month then we must consider dropping the site, or calling for a new maintainer.
    9. Adempiere citizens can call for vote on closing a site if there are at least 3 persons that think it's being abused or not following fair use practices.
    10. Name of the subdomain must be previously discussed with community to reflect the goal and status of the site
    11. Citizens can ask to add or cut advertising on the subdomain sites (via voting process)