Static Code Review and Refactoring

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). 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 app, 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.

Submitting your Work

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

Make sure the Title of your Merge Request is of the form "lab_cellsociety_refactoring - NetIDs".

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 Design Principles for this project and the issues found by the static analysis app 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
   • It is fine to accept this Merge Request whenever your team is ready

Principles

Individually, share your interpretation of the Open-Closed and Liskov Substitution Design Principles and how they apply to the abstractions you have created in the project.

As a group, for each current abstraction in your project, discuss how it helps close a core algorithm in your project and how its subclasses are designed to be substitutable.

Additionally, discuss three new abstractions that would be helpful in light of the given change requirements.

Refactoring

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 course's Code Habits 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:
  • Make Active Classes that use their instance variables instead of giving them away — rein in the get/set methods
  • 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:
  • Methods should return or take as parameters abstractions such as List, Set, Map, Collection, or even Iterator rather than concrete types such as ArrayList or HashMap
  • 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
  • NO downcasts to specific subclasses or instanceof checks (except in equals() methods)
    NOTE, downcasting from a general Object returned from Java to a known abstraction is often required, but not to concrete subclasses
    
• MVC. The model should not depend on the view:
  • Backend classes should throw an exception, rather than return null or simply call printStackTrace(), and frontend classes should catch those exceptions
  • NO references (i.e., import javafx.XXX) to OpenJFX classes within your model classes (not even javafx.scene.paint.Color or javafx.geometry.Point2D)
  • ALL colors, spacing, fonts, etc. must be defined with CSS style properties instead of hard-coded within Java
  • 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.

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