Static Code Review and Refactoring

The interface of a class is the one place where we assert all of the assumptions that a client may make about any instances of the class; the implementation encapsulates details about which no client may make assumptions. — Grady Booch

SonarQube combines a number of static code analysis tools to automatically generate issues by looking for certain features in your code (such as public instance variables) and we have created an app that helps organize and prioritize those issues to clarify why these issues are relevant to your design. It should be noted that this analysis in no way guarantees your code is well designed, it only helps find obvious potential flaws and brings them to your attention. For example, it is an obvious design flaw to have public instance variables, but you can still have a bad design even if all of your instance variables are private. While you are not required to remove all issues found by this tool, you should be prepared to justify why you left significant issues in your code (e.g., with comments near the lines flagged as issues in any of the reports).

As a team, use today's lab time to focus only on the project's design rather than adding new features, focusing your refactoring efforts on:

• removing duplicated code
• fixing basic design issues in your code
• refactoring your code to adhere to assignment specific Design Goals
• reducing the size of your longest methods or classes to follow the Single Responsibility Principle
• adding abstractions to help make the core parts of your design follow the Open Closed Principle
• removing special cases to help your abstractions follow the Liskov Substitution Principle

You will know your design is on the right track if your code:

• Refers to abstractions more than concrete classes directly (especially abstractions your team created)
• Relies on polymorphism when calling methods rather than conditional logic based on a concrete class's "type"
• Uses subclasses to make it clear and easy to determine where to write feature variations

Lab Workflow

In your Cell Society teams, complete the following activities today:

1. Individually, before starting, make sure everyone has committed their latest changes on their branches so there is no possibility any work is lost
2. As a group, discuss the issues found by the static analysis tool and how to approach refactoring the code to address the most important issues
• Summarize your thoughts and priorities in a file called REFACTORING_DISCUSSION.md in the doc folder using Gitlab's markdown format
3. As a group, collaboratively refactor your team's highest priority issues
   • Create a new branch for your work in your team's repository called refactoringLab
4. On Gitlab, create a Merge Request from your refactoringLab branch to your team's master branch so the changes can be compared and approved before being integrated

Submission

At the end of class, use Gitlab's Merge Request to submit your group's refactored code from its separate branch, including your doc/REFACTORING_DISCUSSION.md file to the team repository.

Make sure the Title of your Merge Request is of the form "lab_refactoring - everyone's NetIDs".

Refactoring

Refactoring is the practice of updating a program to improve its design and maintainability without changing its current functionality significantly. An example of refactoring is creating a single method or class that replaces two or more sections of similar code because it reduces the amount of redundant code within the program and makes the code easier to debug and test. Since your project and team size has increased, we have created a tool that helps highlight basic design flaws to give places to start your refactoring. Note, IntelliJ also tries to do this with its yellow warning marks on the right edge of the editor window, but they tend be "lower level" than the ones SonarQube finds.

Examine the results of SonarQube's static analysis of your most recent master branch and, as a team, discuss how fixing each issue would improve the overall program design as well as which issues you think are the most important to work on first. Some issues may have easy fixes and some may require more thought or require more sweeping changes. For example, as we have discussed in class, sometimes removing duplicated code is as simple as creating a new method, sometimes it requires creating an inheritance hierarchy, sometimes generics is the right approach, or a new class or abstraction may be required.

To begin discussing the code's design, review the top issues found in your code using the following steps and record the results in your discussion file:

1. List as many design issues as you can in the method associated with the issue (using line numbers to identify the exact code) from large things like (potential) duplicated code or if statements that should be generalized through polymorphism or data structures down to medium sized things like code structure or unwanted public variables or methods to small things like magic values or inconsistent code.
2. Organize this list of issues based on things that could be fixed together based on a different design choice or using similar refactorings and prioritize these groups based on which would provide the most improvement to the overall code (not just this method).
3. Describe specific overall design changes or refactorings to fix the five most important issues you identified. Note how each change will improve the overall code design, what external changes others will have to make, and what, if any, alternatives you considered. Use Design Principles and Code Smells to justify the goals of your refactoring efforts.

Note, the top issues on the Dashboard that are organized roughly in the following order:

• Longest Method Refactoring. The top issue(s) are typically the Longest Method Code Smell because they can reveal more than just centralization of your code — they are often also the source of inflexibility in your code due to an object doing too much. Look beyond simply breaking these methods up into smaller pieces to see if too much is being handled in a single place and have a wider discussion about how to better organize the code overall.
• Duplication Refactoring. The second issue(s) are typically Duplicated Code, which shows a side by side comparison of places in your project that contain duplicated code, because it is number one in the Code Smell stink parade.
• Checklist Refactoring. The next top issues are a variety of possible coding issues related to the project's Design Checklist.
• Code Smells. The next top issues are a variety of possible coding issues related to Code Smells.
• Java Notes. The next top issues are a variety of possible coding issues based on Joshua Bloch's Effective Java book which describes typical issues in Java code that should be fixed in a standard idiomatic way.

For each of these kinds of issues, you can also see the complete list of issues in your project rather than just the top ones by selecting a specific file, colored and sized by how many issues it has, or by the category option at the bottom of the Dashboard (in its footer).

In your discussion file, describe why you chose to refactor the code as you did and what, if any, alternatives you considered.

Project Specific Issues

As a reminder of some of the project specific design goals, here are a minimal set that every team is expected to reach:

• Encapsulation. The grid data structure's implementation must be hidden:
  • NO public instance variables (also note, protected instance variables also break encapsulation and should be avoided, but public ones are simply poor design)
  • NO explicit public references to grid's data structure implementation (i.e., as a 2D array or a List of Lists) should appear in any public method signature
    Note, ideally, your grid class should implement an immutable interface that is used by the View classes to display the results of the simulation
• Abstraction. Support Open/Closed principle by creating effective inheritance hierarchies that have generally usable methods:
  • NO downcasts to specific subclasses or instanceof checks (except in equals() methods)
• ONE (at most) chain of conditionals (i.e., if or switch) per inheritance hierarchy can be used in the entire project and only for purposes of calling new
  Note, ideally, any such conditional chains should be in a separate method or class, not in the general control flow of existing methods
  
• MVC. The model should not depend on the view:
  • NO references (i.e., import javafx.XXX) to OpenJFX classes within your model classes (even javafx.scene.paint.Color)
  • ALL text displayed in the GUI, from buttons to menus to sliders to error messages, must be keyed from resource properties files using String.format() for dynamic information rather than '+'
    Note, ideally, even your property files should be separated into appropriate packages to further clarify this separation of concerns
• Packages
  • THREE packages (at least) must be used (such as, but not required to be, model, view, and configuration)
    Note, while they do not need to named that specifically, it does need to be clear which packages belong to the frontend and which to the backend

Not every team will have all of the issues above, but every team has at least one of these issues.