Lab Code Review Exercise : Static Code Review and Refactoring

In this lab, focus only on refactoring your program to improve its design rather than adding new features.

In your Cell Society teams, complete the following steps:

1. Review Design Principles
2. Refactor from Static Code Analysis
3. Refactor from Design Requirements
4. Submit Your Work

Before starting:

• Make sure each person has committed their latest changes on their branches so there is no possibility any work is lost
• Create a new branch for your work in your team's repository called refactoringLab

Review Design Principles

Discuss your understanding of Design Principles by having each person share their interpretation of the Open-Closed Principle and the Liskov Substitution Principle. Provide a brief summary of this discussion in REFACTORING_DISCUSSION.md.

Then, choose three important abstractions (inheritance or interface hierarchies) in your project and discuss:

1. Which, if any, of those principles the abstraction embodies (i.e., how it helps close a core algorithm in your project or how its subclasses are designed to be substitutable)
2. If the abstraction could better embody those principles

Finally, brainstorm three new abstractions that would be helpful to create in light of the given change requirements.

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

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

Before Moving On: add your names, shared Design Principle understanding, three current abstractions, and three new abstractions to the doc/REFACTORING_DISCUSSION.md file, then commit and push this file to refactoringLab branch on the remote repository.

Refactor from Static Code Analysis

Static code analysis tools automatically generate issues by looking for certain features in your code (such as public instance variables or JavaFX classes in your model package). We have used Gitlab's CI Pipeline to scan your code each time you push it in an effort to give you immediate feedback on easily detectable design issues. Note that this analysis in no way guarantees your code is well designed — it only brings obvious potential flaws 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.

You can access the report by visiting your project repository hosted in the course's Gitlab group and clicking on the red "X" or green check mark above your list of files (next to the Hex ID of the latest commit). On the next page, click on the "Code Quality" tab to show the list of issues found in the project code. The URL of the resulting page should look like this:

https://coursework.cs.duke.edu/compsci308_2024spring/cellsociety_teamNN/-/pipelines/NN/codequality_report

Planning

In this section of the lab, you will go through violations and address them by refactoring together as an entire group.

Examine the results of the static analysis of your most recent labRefactoring branch (it should be the latest Pipeline since you just pushed your discussion summary). You can review those results by first selecting the refactoringLab branch and then clicking the red ‘X’ or green check.

In the order given below, look to see if you have any of the following violations that usually indicate a significant design issue.

• NPathComplexity
• CognitiveComplexity
• CyclomaticComplexity
• DontRepeatYourself
• AvoidDeeplyNestedIfStmts
• ExcessiveParameterList

If you find one of these violations, open the file to where it occurs by clicking the link and spend time studying the code as a whole using these questions as a guide to addressing this violations.:

• Understanding the design issues
  • Are there any other static code issues associated with this method (refer to the pipeline quality checks)?
  • Are they caused by the same design issue or a different issue?
  • Can you identify any violations of Design Principles that were not caught by static code analysis?
  • Can you identify any Code Smells that were not caught by static code analysis?
  • Are there any change requirements that your current design is not open to?
• Planning a multi-step refactoring
  • Would this violation be addressed by one of the refactorings you planned in Part 1 of the lab?
  • Would this violation be addressed by one of the new abstractions you planned in Part 1 of the lab?
  • Could you resolve this violation with a new abstraction you did not plan in Part 1?
  • Could you resolve this violation by extracting a new method(s)?
  • Could you resolve this violation by adding a new method(s) to an existing abstraction?
  • Is there a different approach you could use to resolve the violation?

Taking all of these questions into consideration, decide on a refactored design that addresses the violation and results in the most overall improvement to the whole project’s design (not just the specific method which may simply be a symptom of more significant, widespread, design issues!).

Execute Multi-step Refactoring

As a team, work together to preform the refactoring needed to address the violation. Note, it is rare that a single refactoring (such as Extract Method or Encapsulate Field) will fix a complex issue or fulfill a Design Principle. When chaining together multiple refactorings it is important to have a plan with small, deliberate steps that keeps your goal in mind so you do not get "lost" on the way and stuck in with non-working code.

After the team feels the complete design issue is handled: commit your changes with a commit message that references the Design Requirement ID represented by the code change. Then, push your refactored code to the remote refactoringLab branch. This will trigger a new pipeline, which will rerun the static code analysis, and allow you to see the effect of your changes on your design immediately (make sure to note if your changes created new significant violations).

Once you have refactored the code that created that violation, repeat this part until none of these issues are present (as a reminder, you will need to address all of these issues eventually through refactorings, comments, or your post-project Analysis Journal).

Refactor from Design Requirements

Along with the violations above that are normally associated with significant design issues, each project needs to adhere to the CellSociety project Design Requirements.

Below is an elaboration of specific minimal implementation details that arise from these requirements. Go through each of these and check if your project meets it. Note violations and where they occur (see Rules in parentheses for static code analysis rules that can help you identify some of these issues), then plan your refactorings using the questions given above and execute them.

• 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
    Rules: ArrayIsStoredDirectly, MethodReturnsInternalArray
    
• 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
Rule: LooseCoupling
  • 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)
    Rule: AvoidJavaFXImportsInModelPackages
  • ALL colors, spacing, fonts, etc. must be defined with CSS style properties instead of hard-coded within Java
    Rules: AvoidLiteralHexColorCodes, AvoidDuplicateLiterals
  • 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 '+'
    Rules: AvoidMagicNumbers, AvoidDuplicateLiterals
    NOTE, ideally, even your property files should be separated into appropriate packages to further clarify this separation of concerns

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 main branch. It is fine to accept this Merge Request whenever your team is ready.

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