API Review and Refactoring

The most difficult problem in teaching object-oriented programming is getting the learner to give up the global knowledge of control that is possible with procedural programs, and rely on the local knowledge of objects to accomplish their tasks. Novice designs are littered with regressions to global thinking: gratuitous global variables, unnecessary pointers, and inappropriate reliance on the implementation of other objects. — Kent Beck

As a team, use today's lab time to focus only on the project's design rather than adding new features, using the Design Coach static analysis app:

• removing duplicated code
• reducing the size of your longest methods
• fixing Code Habit Design Issues in your code
• refactoring your code to adhere to assignment specific Design Goals
• improving your use of "advanced" Java features to improve your implementation
• adding abstractions (interfaces and abstract classes) organized via Design Patterns to help make the core parts of your design follow SOLID Design Principles

Use the course's Design Coach tool and IntelliJ to verify your project does not have a easily findable design flaws. While you are not required to remove all issues found by this tool, you should be prepared to justify your decisions if significant issues are left in your code (e.g., with comments near the lines flagged as issues in any of the reports). As a reminder, no automated tool guarantees your code is well designed, it only helps find potential flaws and brings them to your attention.

Submitting your Work

At the end of class, use Gitlab's Merge Request to submit your group's discussion files and your refactored code from the separate branch, refactoringLab, to your master branch

• doc/REFACTORING_DISCUSSION.md
• doc/API_CHANGES.md

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

Lab Workflow

In your SLogo 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 how your APIs have changed as you worked to implement the project
• Summarize your thoughts and priorities in a file called API_CHANGES.md in the doc folder using Gitlab's Markdown format
3. 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
4. As a group, collaboratively refactor your team's agreed upon highest priority issues
   • Create a new branch for your work in your team's repository called refactoringLab
5. 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

API Changes

At the beginning of this project your team defined four APIs: an external and internal set of public methods for each "side" of the project. For most of you this was your first time specifically designing an API and you did not know a lot about the project you were building. Since then you have also learned new Java techniques that you may have incorporated into your design.

Start by reviewing your original API designs (in your doc/plan/DESIGN_PLAN.md file and the interfaces you created to express that design) as well as your current API (represented here as your project's public methods). When looking at the current version of your API you may decide that some methods do not actually need to be public or that a new interface or class would be useful to more clearly separate part of an internal from an external API or to hide an implementation choice or to add flexibility to the code. To help you clarify the design goals of your APIs, you may want to spend some time reviewing your API's design goals and describing your internal APIs to each other (e.g., how to add a new command to the backend or a new View component to the frontend).

As a team, try to determine:

• what changes that have been made to the APIs
• if those changes are major or minor (justify your distinction based on how much they affected your team mate's code)
• if those changes are for better or worse (if for the worse, is there a way to improve it or was the original API overly optimistic)
• if your foresee any significant changes coming in the next few days as you finish the project (based on your experience and the fact that you now know all the features to be implemented)

Recall that one of the requirements for this project is that your team complete a document, doc/API_CHANGES.md, that describes how your APIs have evolved over the course of the project. This exercise is intended to help you determine the content of that document.

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.

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, data structures, or resource files down to medium sized things like poor error handling or long lambdas methods to small things like consistent coding conventions or ignored assignment design requirements (like using Resources instead of magic values).
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 SOLID Design Principles to justify the goals of your refactoring efforts and Design Patterns to help direct those efforts.

Not every team will have all of the issues below, but every team has at least one of the issues below. They represent a minimal set of design goals for this project that every team should be able to reach and, once you have verified that these issues are taken care of, you can use this time to clean up any aspect of the project's design.

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
  • ALL private instance variables
  • 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)
    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 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, ideally, even your property files should be separated into appropriate packages to further clarify this separation of concerns

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