Page tree
Skip to end of metadata
Go to start of metadata

A lot of the NetarchiveSuite source code is written with very compact naming of variables and methods, which doesn't include much information about the purpose of the variable or methods. The result is that the code can become very difficult to read. This has sometimes has been mitigated by adding inline comments. Writing readable code is in my opinion far superior to incomprehensible code riddled with comments.

Concrete example can be found in the HarvestSchedulerTester#testSubmitNewJobsMakesDuplicateReductionInfo method:

public void testSubmitNewJobsMakesDuplicateReductionInfo() {
    Method m = ReflectUtils.getPrivateMethod(HarvestScheduler.class, "submitNewJobs");
    hsch = submitNewJobsAndGetSchedulerInstance();
    //Get rid of the existing new jobs
    m.invoke(hsch);
    ...
}

 A much better way to write this code would be:

public void testSubmitNewJobsMakesDuplicateReductionInfo() {
    clearExistingJobs();
    ...
}

/**
 * Clear the existing jobs by running a <code>submitNewJobs</code> on the scheduler.
 */
 private void clearExistingJobs() throws Exception {
    Method submitNewJobMethod = ReflectUtils.getPrivateMethod(HarvestScheduler.class, "submitNewJobs");
    submitNewJobMethod.invoke(harvestScheduler);
 }

The following changes has been made:

  • The 'Get rid of the existing new jobs' implementation and documentation has been encapsulated in a dedicated method. The method is reusable so all the duplicate occurrences of the first code snippet can be replaced by a clearExistingJobs();
  • The m variable has been renamed to a more information name.
  • hsch variable has been renamed.
  • The hsch variable assignment has been moved to the setUp method to be symmetric to the usage at the tearDown level. The HarvestScheduler is a singleton, so there is no need to wait until the concrete test method is called. The functionality of the HarvestScheduler should be not be dependent upon the exact instantiation time (singleton should be robust towards such invariants), so the the individual unit tests assume they have control over the HarvestScheduler instantiation, and therefore first scheduled run.

This is much more readable piece of code IMHO.

  • No labels

3 Comments

  1. I don't understand the following text. Is something missing here?

    " Writing readable code is in my opinion is fare superior to incomprehensible riddled with comments."

  2. Your example comes from the test-code, which have never been our primary focus.

    1. Yes, I've just added a new topic Reviewing test code, where I try to argue that it might be an good idea to lift the quality of the test code by including it in the normal review process. 

      I used the code from the unit test because the most pronounced anti-patterns can be found here, but elements of the same problems can be found in the main code.