This Confluence has been LDAP enabled, if you are an ASF Committer, please use your LDAP Credentials to login. Any problems file an INFRA jira ticket please.

Child pages
  • Re-Factor To-Do List

Access to add and change pages is restricted. See: https://cwiki.apache.org/confluence/display/OFBIZ/Wiki+access

Skip to end of metadata
Go to start of metadata

This page contains the current list of the tasks that need to be done as part of the Framework Re-Factor. The goal is the allow volunteers to select a task and provide a patch that will resolve each of the tasks listed. The list of tasks have three degrees of complexity (Difficult, Moderate and Easy) allowing contributors at any level to participate.

To pick up any of these tasks

  1. Add your name to the "Who is working on it" column
  2. Create a JIRA Issue for the task (e.g use the name or area as the title and copy the what needs to be done into the issue description)
  3. Remember to use the trunk as the code source for your re-factor and patch

In order to improve the quality of what is being re-factored, please note that for each task the developer should also:

  • Write unit tests for every line of code being re-factored.

 

REFNAME OR AREAWHAT NEEDS TO BE DONECOMPLEXITYSTATUSWHO IS WORKING ON IT?JIRA MAST ISSUE LINKCOMMENTS
1remove dead importsRemove all unnecessary import statements in all java filesEasyFixedMichael Brohl OFBIZ-6971 - Getting issue details... STATUS  
2Fix resource leaks in CommonServices.javaFile1 and file2 should be fixed by closing them in byteBufferTestEasyFixedMichael Brohl OFBIZ-6972 - Getting issue details... STATUS  
3Remove deprecated dependencies in EmailServicesorg.ofbiz.common.email.EmailServices.java depends on FoScreenRenderer and HtmlScreenRenderer. Both should be replaced by MacroScreenRenderer.ModerateFixedDeepak Dixit OFBIZ-5780 - Getting issue details... STATUS  
4Simplify getChildHRCategoryTreeThe method getChildHRCategoryTree in org.ofbiz.humanres.HumanResEvents is too long and complicated. The business logic can be considerably reduced and simplified. The function should be broken down into multiple private functions to simplify the calls and to create the right level of abstractionModerateFixedKulwant Singh OFBIZ-6986 - Getting issue details... STATUS  
5Add Generics to PaidInOutIn ofbiz.pos.screen.PaidInOut.java the DefaultComboBoxModel is not parameterized. Need to put the right generics in placeModerateIn AtticJacques Le Roux OFBIZ-7804 - Getting issue details... STATUS  
6CommonWorkers.java needs simplifying and cleaning up

Refactor CommonWorkers.java. Lots of code is repetitive and can be factored out into shared private methods. This includes things like:

  • Exception handling
  • Queries
  • Building the return data structures
ModerateIn ProgressMartin Becker OFBIZ-6984 - Getting issue details... STATUS  
7Cleanup FindServices.java

Many problems exist in this file:

  • Commented out code should be removed
  • File is too big, need to be broken down
  • Ugly business logic in most of the functions which can be simplified considerably
  • Delegator code needs to be isolated and side effects kept down to a minimum
Moderate Malin Nicolas OFBIZ-6974 - Getting issue details... STATUS  
7bRefactoring permission service

Homogenise the call to permission service by ModelService.

Clean deprecated code

ModerateIn Progress Malin Nicolas OFBIZ-7113 - Getting issue details... STATUS  
8Parameterize everything in DebugManagedDataSourceGenerics need to be introduced to multiple places. Requires knowledge in commons-poolModerateIn ProgressMartin Becker OFBIZ-7039 - Getting issue details... STATUS  
9XML shared dependencies between accounting and HRMany shared dependencies exist bi-directionally between accounting and HR including screens, entities and other items. All such XML should propagate down to the commonext componentModerate Pranay Pandey OFBIZ-7208 - Getting issue details... STATUS  
10Redesign EntitySaxReader

org.ofbiz.entity.util.EntitySaxReader requires refactoring to achieve the following goals

  • Refactor EntitySaxReader to an interface
  • Create an implementing class
  • Remove all dependencies on Javolution
  • Fix all dependencies in the framework to match the new signature of the interface
  • Delete the javolution library from the framework
DifficultIn ProgressMartin Becker OFBIZ-7040 - Getting issue details... STATUS  
11Redesign org.ofbiz.entity.datasourceAll objects under org.ofbiz.entity.datasource need to be redesigned into an interface model and implemented with concrete classes. Things like the GenericDao should be broken down to many pieces as it is massive, complex, and overly designed (Interface Segregation principle violated)Difficult 

Mridul Pathak

Divesh Dutta

  OFBIZ-7210 - Getting issue details... STATUS  
12XML shared dependencies between accounting and orderMany shared dependencies exist bi-directionally between accounting and order including screens, entities and other items. All such XML should propagate down to the commonext componentDifficult Pranay Pandey OFBIZ-7207 - Getting issue details... STATUS  
13Start.javaThis has some problems which are being tackled in JIRA OFBIZ-6783DifficultFixedTaher A. Alkhateeb OFBIZ-6783 - Getting issue details... STATUS  
14Enforce noninstantiability to all Utility classesDesign flaw needs to be fixed on all Utility Classes, discusses over mailing list.ModerateFixed

Rishi Solanki

Arun Patidar

OFBIZ-7272 - Getting issue details... STATUS All
15harmonise permission applications in widgets

Many permission definitions are hard coded in widgets (e.g. menu-items and screens), e.g.

    <condition>
        <if-has-permission permission="ACCOUNTING" action="_ADMIN"/>
    </condition>

Many of these kind of 'default' permissions definitions could be harmonised by application of the ${activeApp} variable.

Moderate Pierre Smits  
16Refactoring UIReview and improve the UI for more flexibility and user friendly, see dedicate wiki pageDifficult ManyUI Improvement 

 

Still want to help but in other areas ?

If our current re-factor to do list seem a little too much for you to take on then you can help in other areas too, and a little bit of work quickly adds up.  Please take a look below for some other ideas for helping remove clutter and help clean up the code base.

  • Obsolete / irrelevant comments: anything out of date
  • Commented out code:  it belongs in the version control system
  • Functions with too many arguments: anything beyond 3 arguments is probably too much unless there is a good reason for it.
  • Functions that do too many things - multiple languages in the same text file
  • Big files / Big classes / Big anything really!
  • Duplication and cut-and-paste patterns
  • Mixed levels of abstraction: You can't declare high level stuff like starting the framework with details like flag parsing in the same place. Things should read like a story from high level down to the details. Main calls higher level functions which then call lower functions which executes detailed code.
  • Any concrete class not implementing an interface is probably a code smell, especially if too many dependencies point to it.
  • Cluttered code, sandwiched in an ugly way
  • Pretty much all the Java warnings in the current code base
  • Too much use of the "new" keyword instead of having a proper factory
  • Writing to classes instead of interfaces - Confusing names for classes, functions, and variables. Things should be very clear and simple
  • Confusing test names - Lack of testing for any production code. Ideally, our tests should cover 100% of the code base.
  • Inconsistent formatting conventions. Tabs instead of spaces, wrong number of spaces for indentation, and so on
  • Also, one of the worst things I usually encounter is hidden state. For example, you get hidden state in a Java class if the constructor declares a field that was not passed into the constructor. It makes the declaration hidden and the dependency obscure.

 

 

IMPORTANT: If you have any questions about any of these tasks or need feedback on a proposed patch then please post a message on the development mailing list.

  • No labels

16 Comments

  1. Re: item #9: XML shared dependencies between accounting and HR +  item #12 XML shared dependencies between accounting and order

    The referenced components (accounting, humanres and order) are not in the framework stack. Or is there a redefinition of 'framework' that I am not aware of?

    1. Hi Pierre,

      The refactoring exercise is initiated really for the entire thing, not just the framework sub directory. Everything can benefit from refactoring and cleaning up.

      We encourage you to take any code in any component in which you are comfortable and add it to the sprint. The purpose of this page is to initiate awareness in the community of the importance of sustaining good quality code anywhere and everywhere.

      Please consider pitching it if you have time, we need help from everyone.

  2. Re: item #5 Add Generics to PaidInOut

    This has little to do with 'framework'. The pos component is in the special purpose stack. 

    1. Hi Pierre,

      Please check my comment above

  3. I don't see any reference to javadoc?

    The code seems to lack in-line documentation of classes and methods in most of the code that I looked at.

    1. As Taher suggested feel free to create a new line in the table... (smile) Maybe this will need to be more detailled though, anyway baby steps as always...

      BTW this is a good article to read about code documentation

    2. Hi Ron,

      Task 14 added by you is too massive and unrealistic. I suggest either to move it to the comments below the tasks or redefine it to something more specific. Small baby steps as Jacques mentioned is the key for the refactoring project to work.

      The table above represents a list of tasks to execute in a sprint, not general guidelines or mega projects. those should be added elsewhere in this page

      1. BTW Ron, you also mentionned test coverage. As an insider, I believe this is at least as important as a good documenation. I'd suggest to create specific children pages for test coverage and documentation. In these pages people could create a table with detailled steps as here

      2. It is not really intended to be a massive task on its own.

        It is intended to be a sub-task integrated into every other task that touches an interface (or class)  that should be documented.

        Any task that creates a new interface and refactors classes that are not referenced through an Interface should not be committed without proper documentation.

        "If you are in a hole, the first thing to do is STOP DIGGING."

        Accepting a bunch of code that is not documented or has inadequate test coverage is just adding to the technical debt of the project.

        1. Hi Ron,

          If it is something to be done in any task, then it is not itself a task but a guideline; hence it should be removed from the list. Again, the above list is doable, closable list of tasks that we can apply, once we complete them we create another batch. This task cannot be "taken" by anyone or assigned a JIRA as it is a general guideline.

          So I suggest to please remove it from the table and put it in the Coding Practices page

          1. Good point. I removed it.
            If someone comes across any classes that are important and missing JavaDocs that are not part of a refactoring project, I guess that would be a candidate for inclusion as a task.

            Same with test cases.

            We depend on the team accepting commits to verify that the JavaDoc comments are included and that test cases are provided before accepting code into the refactored code base.

             

          2. Actually, such can be a JIRA issue (e.g. as a placehoder description)  that can be associated (linked) to any other JIRA issue that is appropriate. For each of the other issues it will show the link. And when contributors take on such an issue, the can get a grip on what needs to be taken into consideration before committing the change.

            1. Hi Pierre,

              This is not applicable for this project. We designed it to be in multiple sprints, each holding a list of "achievable" tasks. This project is completely focused on bite-sized achievements, not "Big" tasks. You can check the exchange above.

              1. Agreed but it seems to me that Pierre's comment describes a method of helping breakdown the projects into chunks that can be tackled by a team.

                For example, if you are working on a top level task and want some help to develop an adequate test suite, you can create a sub-JIRA identifying the test cases that you think need testing so that others can work on those while you proceed with the refactoring.

                Documentation is harder to work on since the person who is working on the main refactoring is probably the best person to describe the functionality.
                However, it would be reasonable to make a sub-JIRA asking for help in documenting a set of the trivial methods or trivial dependency classes associated with the classes being refactored. These may do not require a lot of re-engineering of code but still take more time than the principal investigator has available.

                The key point as far as I am concerned is to stop adding to the technical debt or worse yet create situations where people modify the code in the future in ways that are less robust or just wrong simply because they have no documentation or test cases to show what it is supposed to do.

                Also we do not want to end up with good code that get refactored in the future because there is no documentation or test cases so future investigators assume that it will be easier to rewrite the code than reengineer it so they can modify it.

                The proper use of sub-JIRA's will preserve the investment made by the principal investigator in understanding the functionality of the code under consideration while generating as much of a team effort as possible.

                if the sprint makes the technical debt greater or reduces the maintainability of the code, it will have failed regardless of how much "better" the code is.

  4. I read the article.

    He seems to focus on application code rather than libraries.

    As we are focusing on the framework, we need to focus on the use of comments in helping people understand what will happen if they use the methods.

     

    Comments clearly do help even in application code as it gets debugged and modified.

    For libraries, good comments are essential in ensuring that people who use the library, understand the function, input and output of each method.

     

    It also helps people maintaining code to understand the contract that has been in place between the author and the other programmers who have used the class and its methods.

    This should help to ensure that improvements to the code does not break client applications without warning. 

    It can also give some hints about whether a method can be upgraded or must be replaced and marked as deprecated until applications can be modified to use the "better" method.

     

    1. Actually the main point of the article is to emphasis the importance of what he calls

      The 2 first types of comments are most interesting when commenting an API (aka a lib) because they define how developpers can use it.

      As he concludes:

      Contracts should be seriously considered but the level of detail should be chosen in relation to the intended reuse. Last but not least, deliberate context comments are a keeper.

      There are already such documentations in OFBiz but clicking on the 1st classe (AbstractCache) clearly shows that we can do better. There are good examples in Java code of course, but also in most other Apache projects
      BTW no needs to be a developer to tackle such tasks...