Skip to end of metadata
Go to start of metadata

You are viewing an old version of this page. View the current version.

Compare with Current View Page History

« Previous Version 4 Next »

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 is fare 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.
 *
 * @throws Exception
 */
 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