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 SLogo teams, complete the following steps:

1. Review Design Principles and Patterns
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 and Patterns

Discuss your understanding of Design Principles and Design Patterns by having each person share their interpretation of the Interface-Segregation Principle and the Dependency Inversion Principle. Provide a brief summary of this discussion in REFACTORING_DISCUSSION.md.

Then, each person choose a Design Pattern that you think already exists in your project or could help improve the design in light of the given change requirements and discuss:

1. Which classes collaborate to implement the Design Pattern?
2. How does your existing or planned implementation differ from the canonical Design Pattern?
3. What trade-offs were or should be considered in choosing the Design Pattern?

Finally, if any of the Design Patterns are not yet implemented, briefly sketch what refactoring steps would be necessary to implement it.

Before Moving On: add your names, shared Design Principle understanding, four Design Patterns, and possible refactorings 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/slogo_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 project Design Requirements.

As a reminder, 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

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 implementation must be hidden:
  • Make Active Classes that use their instance variables or pass them to Lambda created classes instead of giving them away — rein in the get/set methods
  • ALL private instance variables
  • ALL model.api classes should be abstractions, immutable, or Exceptions
    NOTE, prefer implementing many small interfaces that can be passed to different sets of classes, rather than the one interface or the entire concrete class
  • NO duplication in command subclasses (such as common argument processing, common error handling, or looping over active turtles, etc.)
• 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
  • NO chain of conditionals (i.e., if or switch) can be used in the entire project
  • NO downcasts to specific subclasses or instanceof checks (except in equals() methods), especially in the Parsing algorithm
    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)
  • NO hardcoded references to fixed number of languages, images, or configurations supported (i.e., direct constants for languages or property files)
  • 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, 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_slogo_refactoring - NetIDs".