#Testing and Refactoring - Alitheia Core In this report we describe and discuss the refactoring changes made to Alitheia Core and present a plan for future refactoring. The most important part of the refactoring of Alitheia Core is to make it more maintainable and attractive for developers to use and develop plugins.
The report is structured as follows. First we discuss the provided test suite, then we describe the refactoring processes of various Alitheia Core components. The report is finalized by summarizing the overall progress made and giving future recommendations regarding further refactoring of Alitheia Core.
##Provided Test Suite The provided test suite set up in the project did not work as expected. There were a couple of issues:
eu.sqooss.test.service.scheduler.SchedulerTestruns forever because the tearDown method is stuck in an infinite loop. The Scheduler test cases do not get along with the Scheduler implementation because the test cases assume that the scheduler have no waiting jobs after an invocation ofstopExecute, which is not the case. We fixed this by checking the amount of worker threads instead of the waiting jobs.eu.sqooss.admin.test.AdminServiceImplTestfails randomly. The cause of this erratic behavior is that JUnit will execute the test cases in random order. JUnit is doing perfectly fine because unit tests need to be independent of each other in order to reduce complexity in the test suites. We fixed this issue by separating the logic in two different classes and using the@BeforeClassannotation to ease the setup.
The following section will contain the most important modules we refactored. We will outline the approach we took for refactoring the modules and we will describe test cases.
To start off and familiarize ourselves with the testing process we tested and refactored a relatively small class: PluginInfo. This was done using JUnit tests. In order to not fully initialize the full program, we set up various mocks and created a custom testing BundleContext.
By creating our test suite we found that the values of the String ConfigurationType are not type checked, this could potentially lead to problems when a null value is put into the database. We are unsure if this is wanted behavior, but to make sure we will create an issue and ask the developers in the main fork.
In the refactoring phase, the fromString function for the enum ConfigurationType was simplified. Java already has a function called valueOf for enums, which basically performs the exact same task as the fromString function. The only main difference was that it would return a null value, but this value was always checked in all the callees and an exception would be thrown if it was null.
Thus the possibility for the return type to be null was removed and the fromString function was made responsible to throw the exception. This reduced code duplication in the file, giving a better overview. The main problem can be seen in the figure below. It is important to note, although this is a mediocre case, that it previously would take more time to extend an enum to contain more members and it would introduce bugs more easily.
Various other minor refactorings were made to the class, but we believe that the development team should focus more on code-reuse, such that existing methods are used instead of re-implementing already available functions. There are numerous enums which for which this fromString is implemented. These recommendations will be discussed in detail in the Future Recommendations section.
###ProjectVersion The ProjectVersion file is a large class (~1000 LoC) and has no test cases. This class violates the Single Responsibility Principle because there are 60 different methods performing different actions. We also noticed from the first assignment that there were more than 10 different classes depending on this class, which is a lot. We built some test cases for this class using Powermock and Mockito. The difficult part is that this class mostly contains methods that depend on global singletons such as AlitheiaCore and DBServiceImpl. We mocked these classes in order to test some basic interaction with the database. We did not solve the SRP problem for this class because separating the class into an interface would put a burden on the communication with the database. This is because the classes are generated by JAXB, changing a bit in the database will automatically generate the corresponding methods. Refactoring this to an interface will mean that we loose this featuer. Furthermore we did not develop test cases for all methods because some methods were so trivial, that mocking the database access would not be useful. It would mock the response and verify that the response is the same as expected, which isn’t a real test case in our opinion. We found some small bugs where certain branches could never be reached and fixed these.
###PAServiceImpl
The error-handling in PAServiceImp was cluttered, as it relied mainly on Boolean evaluations. This form of error-handling was replaced by the usage of exceptions. Not only does this give a better overview, but it also provides the logger with more detailed error messages. Some other refactorings such a method extraction were performed as well. The logic of this class remained the same, there was only extraction of methods, which does not have a direct effect on the code functionality, but improves code readability. Compatibility with the rest of the components in the system remains, as the install function will still return a boolean depending on its success or failure.
We began testing this class, but it soon turned out that this god-class, which violates the Single Responsibility Principle, should be refactored into smaller more manageable pieces. Within the given timeframe, we were unable to fully refactor this class and leave this to future work.
###FileUtils and DiskUtils
These two utility classes were re-implementing methods for which existing frameworks can be used. This violates the Single Responsibility Principle, not on a class level, but rather on a component level. Implementing these types of methods yourself increases the likelihood of bugs, and clutters the project overview. Thus we refactored all the classes which made use of this, to use the Apache Commons IO library, specifically the org.apache.commons.io.FileUtils and org.apache.commons.io.FilenameUtils classes were replacements for FileUtils and DiskUtils. In order to fully test these changes it would require a lot of time and which would inefficient if the affected classes were not going to be refactored. Thus for these changes to be made, we carefully analyzed the call hierarchy within the whole project, and made the corresponding changes while checking the logic manually. In total this allowed the removal of ~447 lines of code from the Alitheia Core codebase.
###SQO-OSS Admin Panel In the end, the user must be able to add projects, run metrics and print out reports. The current web interface contains bugs and is lacking feedback to the user. Building a robust code analyzer is useless if you fail to interact with the user. There are no test cases for the web interface. There are some tools available for testing web interfaces such as Selenium that could be integrated in Maven to run automated UI tests. In the first assignment we addressed the PluginView class which contains almost 1000 LoC and is clearly violating the Open Closed Principle. The class also contains a large portion of code duplication because all the rendering is hard-coded in Java. This class really needs to be refactored because altering the web interface is almost impossible to do. Even when the table wants another header, i.e. “Listed Plug-Ins” instead of “All plug-ins”, we need to change this at two different places in the code.
Despite the look, Alitheia Core does use a template engine named Velocity Engine from Apache. This template engine is pretty powerful but is hardly used in the project. We want to move every piece of Java code that is generating HTML to the template engine. This is a good idea because different templates and designs could easily be created and we can test the models delivering the data separately. This is a huge task and obviously we can’t do everything in this period of time, so we started by creating another Plugin-in tab with the refactored code, but having the same functionality. In this way we can check by hand if the underlying code is doing the same. The following figure represents the size of the targeted files that were refactored.
Lines of code isn’t a good metric, but we have managed to reproduce the same functionality as the original PluginView with only 10% of the original lines of code. The Open Closed Principle is fixed, we can easily extend the template or write another template for example if we want different designs.
##Future Recommendations In this section we will give some future recommendations for the project. We will give advice on what wee think are the most critical parts of the project and where more refactoring of the project should continue. ###Replace the Scheduler In order to have a fully optimal working cluster, Alitheia Core needs to have a well functioning Job Scheduler. We believe that it would be more optimal if not also safer to use Quartz-Scheduler in combination with RabbitMQ. A design decision should be made for this. If the design team wants to give the ability to do clustering with Alitheia Core, it should be done in combination with the aforementioned tools, or if this is not the case it should be removed from the project as a whole.
Our personal recommendation would be to replace the scheduler with the Quartz-Scheduler in combination with RabbitMQ. As it will improve the way jobs are scheduled right now and improve extensibility of Alitheia Core.
As was seen in the section SQO-OSS Admin Panel, there could be a lot of improvement in the web views. The Velocity Engine is present, but is hardly used. Refactoring all of the views would result in much cleaner (and less) code. Doing this will leave some space for more improvements like adding more attractive designs.
To improve maintainability, as Alitheia Core will increase in size, we believe a large improvement will be if the project gets spread out over multiple projects. The project would then be split up into three sub-projects:
- Alitheia Core : contains interfaces and dependencies shared between the projects.
- Alitheia Core - WebInterface : is resposible for display the user interface by simply reading from the database and querying the cluster manager in order to perform certain tasks.
- Alitheia Core - Cluster Manager : is responsible for the task scheduling for master and slave servers.
- Alitheia Core - Server : has two types: master server and slave server. Which are managed by the cluster manager.
We strongly believe in the prevention rather than mitigation, therefore we would also propose a feedback session with the development team, depending on how active the "legacy" contributors are in the current organization. This of course will be done in a very constructive non-intimidating way, by not using the Alitheia Core project itself, but by using external examples.
##Conclusion During the refactoring we have been able to spot many problems to which we have offered a solution via our pull-request. With an additional 10.000 euro funding for this refactoring process we will be able to perform the future recommendations, such split up this project in more manageable pieces and make the software more appealing for researchers to develop metrics and plugins for. As the project becomes more manageable, it reduces costs for maintainability.
http://i.imgur.com/DjP1t0S.png