Graduate Program KB

Clean Code

Refactoring SerialDate

• Refactor SerialDate in the JCommon library
• We should be comfortable doing professional reviews, we learn from critiques

First, Make It Work

• Tests in SerialDateTestsdon't cover everything

  • A "Find Usages" search on method MonthCodeToQuarter indicates it's not used (unit tests don't test it)

• Clover is a tool to measure code coverage for software testing

  • Only 91 of 185 executable statements were ran in SerialDate

• Apply Boy Scout Rule to this implementation

  • Fix test coverage (write own test suite)
  • Fix bugs (failing tests that will pass as refactoring occurs)
  • Clear and minimal code (refactor)

• With new test suite, Clover reporting 170 of 185 statements being executed

• Some failing tests / bugs:

  • Support for abbreviations "tues" and "thurs" (commented out test cases)
  • Tests for incorrect input errors

  153 //todo assertEquals(-1, stringToMonthCode("0"));
  154 // assertEquals(-1, stringToMonthCode("13"));
  

  • Tests on line 163 to 213 are failing

  163 // assertEquals(1,stringToMonthCode("jan"));
  164 // assertEquals(2,stringToMonthCode("feb"));
  165 // assertEquals(3,stringToMonthCode("mar"));
  166 // assertEquals(4,stringToMonthCode("apr"));
  167 // assertEquals(5,stringToMonthCode("may"));
  168 // assertEquals(6,stringToMonthCode("jun"));
  
  ...
  
  208 // assertEquals(7,stringToMonthCode("JULY"));
  209 // assertEquals(8,stringToMonthCode("AUGUST"));
  210 // assertEquals(9,stringToMonthCode("SEPTEMBER"));
  211 // assertEquals(10,stringToMonthCode("OCTOBER"));
  212 // assertEquals(11,stringToMonthCode("NOVEMBER"));
  213 // assertEquals(12,stringToMonthCode("DECEMBER"));
  

  if ((result < 1) || (result > 12)) {
      result = -1;
  
      for (int i = 0; i < monthNames.length; i++) {
          if (s.equalsIgnoreCase(shortMonthNames[i])) {
              result = i + 1;
              break;
          }
  
          if (s.equalsIgnoreCase(monthNames[i])) {
              result = i + 1;
              break;
          }
      }
  }
  

  • Boundary error bug in getFollowingDayOfWeek method where it returns December 25th as the Saturday that follows December 25th

  if (baseDOW >= targetWeekday) {
  

  • Code that never gets executed, conditional statement on line 718 always evaluates to false
  • Not returning an IllegalArgumentException instead of returning an error string

Then Make It Right

• With all unit tests passing, it's time to apply TDD while refactoring the implementation

• Some parts to refactor:

  • Leave comments with license information, copyrights, authors, remove change history dating from 1960s and other redundant comments in the code
  • SerialDate is not a good class name, should just be named Date (DayDate was chosen as a compromise)
    • Class is abstract, "Serial" implies an implementation
    • "Serial" is not the correct term for its representation, serial usually means product identification markers than dates
  • Base classes shouldn't know about their derivatives, use the abstract factory pattern and create a DayDateFactory
  • Move Day enumeration into its own source file
  • Removed flags passed to functions
  • Extracting or deleting unnecessary functions during refactoring

• Code coverage decreased but misleading, 45 of 53 executable statements covered by tests

  • The uncovered lines are trivial, not worth testing
  • Class has shrunk so percentage-wise, the uncovered lines have greater weight

• Overall, test coverage increased, some bugs fixed and code was clarified and shrunk

• Despite the code being improved, the next person to view this code could also try to clean it up a bit more (Boy Scout Rule)