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
- Add your name to the "Who is working on it" column
- 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)
- 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.
REF | NAME OR AREA | WHAT NEEDS TO BE DONE | COMPLEXITY | STATUS | WHO IS WORKING ON IT? | JIRA MAST ISSUE LINK | COMMENTS |
---|---|---|---|---|---|---|---|
1 | remove dead imports | Remove all unnecessary import statements in all java files | Easy | Fixed | Michael Brohl | ||
2 | Fix resource leaks in CommonServices.java | File1 and file2 should be fixed by closing them in byteBufferTest | Easy | Fixed | Michael Brohl | ||
3 | Remove deprecated dependencies in EmailServices | org.ofbiz.common.email.EmailServices.java depends on FoScreenRenderer and HtmlScreenRenderer. Both should be replaced by MacroScreenRenderer. | Moderate | Fixed | Deepak Kumar Dixit Dixit | ||
4 | Simplify getChildHRCategoryTree | The 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 abstraction | Moderate | Fixed | Kulwant Singh | ||
5 | Add Generics to PaidInOut | In ofbiz.pos.screen.PaidInOut.java the DefaultComboBoxModel is not parameterized. Need to put the right generics in place | Moderate | In Attic | Jacques Le Roux | ||
6 | CommonWorkers.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:
| Moderate | In Progress | Martin Becker | ||
7 | Cleanup FindServices.java | Many problems exist in this file:
| Moderate | Malin Nicolas | |||
7b | Refactoring permission service | Homogenise the call to permission service by ModelService. Clean deprecated code | Moderate | In Progress | Malin Nicolas | ||
8 | Parameterize everything in DebugManagedDataSource | Generics need to be introduced to multiple places. Requires knowledge in commons-pool | Moderate | In Progress | Martin Becker | ||
9 | XML shared dependencies between accounting and HR | Many shared dependencies exist bi-directionally between accounting and HR including screens, entities and other items. All such XML should propagate down to the commonext component | Moderate | Pranay Pandey | |||
10 | Redesign EntitySaxReader | org.ofbiz.entity.util.EntitySaxReader requires refactoring to achieve the following goals
| Difficult | In Progress | Martin Becker | ||
11 | Redesign org.ofbiz.entity.datasource | All 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 | ||||
12 | XML shared dependencies between accounting and order | Many shared dependencies exist bi-directionally between accounting and order including screens, entities and other items. All such XML should propagate down to the commonext component | Difficult | Pranay Pandey | |||
13 | Start.java | This has some problems which are being tackled in JIRA OFBIZ-6783 | Difficult | Fixed | Taher Alkhateeb | ||
14 | Enforce noninstantiability to all Utility classes | Design flaw needs to be fixed on all Utility Classes, discusses over mailing list. | Moderate | Fixed | All | ||
15 | harmonise 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 | |||
16 | Refactoring UI | Review and improve the UI for more flexibility and user friendly, see dedicate wiki page | Difficult | Many | UI 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.
16 Comments
Pierre Smits
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?
Taher Alkhateeb
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.
Pierre Smits
Re: item #5 Add Generics to PaidInOut
This has little to do with 'framework'. The pos component is in the special purpose stack.
Taher Alkhateeb
Hi Pierre,
Please check my comment above
Ron Wheeler
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.
Jacques Le Roux
As Taher suggested feel free to create a new line in the table... 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
Taher Alkhateeb
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
Jacques Le Roux
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
Ron Wheeler
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.
Taher Alkhateeb
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
Ron Wheeler
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.
Pierre Smits
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.
Taher Alkhateeb
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.
Ron Wheeler
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.
Ron Wheeler
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.
Jacques Le Roux
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:
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...