Skip to content
Snippets Groups Projects
Commit b62f01e6 authored by Chris348794's avatar Chris348794
Browse files

Added README report

parent d9c54ef9
Branches main
No related tags found
No related merge requests found
This codebase was sourced from https://github.com/briggscole/soccer-league-scheduler.git, This codebase was sourced from https://github.com/briggscole/soccer-league-scheduler.git,
which was a group project for CPSC 433 at the University of Calgary. which was a group project for CPSC 433 at the University of Calgary.
Permission was recieved from all related group members Permission was recieved from all related group members
(Cole Briggs, Erioluwa Soyemi, Grace Kelly Osena) to use this codebase for this purpose. (Cole Briggs, Erioluwa Soyemi, Grace Kelly Osena) to use this codebase for this purpose.
\ No newline at end of file
Access has been given to the repo. It is at https://csgit.ucalgary.ca/christopher.axten/a1-refactoring.
My first refactoring concerns the “Slot” class, specifically all methods related to the boolean
variable “isGameSlot”. The problem was that the Slot class was unreasonably large due to many of
its methods having different functionality depending on whether a Slot was a “game slot” or not,
which resulted in many conditional statements. Since much of the Slot class’s functionality was
coupled to this variable, many changes were needed. The changes within the Slot class were
straightforward, and involved removing the previously mentioned boolean variable then extracting
the subclasses GameSlot and PracticeSlot from the functionality of the relevant methods. These
relevant methods include the constructor, getType, duration, getMax, overlaps(Slot),
isSpecialPracticeSlot, isGameSlot and isPracticeSlot. The Slot class itself was made abstract,
along with these methods, and the “isGameSlot” boolean was removed from its constructor’s header.
This removed the need for the conditional statements. The Parser class was the only class that
called the Slot constructor, which consisted of a conditional statement for whether or not to
set “isGameSlot” to true, so this was updated for the new syntax. The code was tested on the
previously mentioned methods in Slot and on the Parser class through the use of 2 test input files.
These changes were made on a new “Game/Practice Slot Refactoring” branch in commit
894f1d4bc04ff78f15a5f84c6a6c993eb27c1619, which was merged back into the trunk. The code is better
structured after this refactoring because it is easier to understand and find the functionality that
is abstract for all slots or specific to game or practice slots. It also suggests that other
refactorings are needed, as the methods “isGameSlot” and “isPracticeSlot” still exist and are used
by other classes.
My second refactoring concerns the “Instance” class and its long parameter list. About half of these
parameters were “penalty” values for evaluating Schedule objects, so I extracted a Penalty class
from the Instance class to replace these penalty values with a single Penalty object in commit
dc49097956440cb3200d1293860bea76ee6c1f23 on a new “Penalty-Class-Refactoring” branch from main.
The Penalty class consists of five integer values for each penalty: gameMin, practiceMin, notPaired
and section. In addition to the Instance class, the Parser and Schedule classes had to be updated
as well. In the Parser class, a single call to the Instance constructor was updated. In the Schedule
class, all calls for the Instance object’s penalty values were updated to call its new Penalty
object’s values instead. The functions that required these updates were evalMinFilled, evalPair and
evalSecDiff. The ParserTest and InstanceTest unit tests from the previous refactoring also had to be
updated for the Instance object’s new parameter list. Unit testing around this second refactoring was
also made, and all these changes were made in commit bae04bcec7cff2983799e9c1aa5fec73b07bcca2. These
tests are specifically around the Instance constructor and any calls for penalty values within the
codebase. The branch was then merged back into the trunk. The code is better structured now as the
Instance parameters are much easier to read. It is now much easier to identity the data values within
the Instance class and how they relate to each other. However, the Instance class’s parameter list is
actually still quite long, suggesting that yet another class extraction could be useful. The Penalty
class is also currently a data class, suggesting that functionality could be moved inside it.
For my third refactoring, the bad code smell was dead code. I removed the methods Instance.printSlots
and Slot.getType. Due to the first refactoring, GameSlot.getType and PracticeSlot.getType further
trivialized the original functionality while spreading it across multiple subclasses, despite the only
other method that calls it being Instance.printSlots. Instance.printSlots in turn appears to be a
leftover from previous debugging, as no other method calls it. Along with these methods were 11 other
print methods that were not called by any other methods, which were also leftovers from debugging, which
were also removed. These methods include Event.printIncompatibleEvents, Event.printUwantedSlots,
Instance.printIncompatibles, Instance.printUnwanteds, Event.printPreferences, Instance.printPreferences,
Instance.printSlots, Instance.printPartialSchedule, Schedule.printSlotStatus, Event.printPairs and
Instance.printPairs. This commit was 66513af4eb5f77daec3216500da088a675f30eab. No test cases were added
for this refactoring, but the unit tests for the Slot.getType method were removed. This refactoring leave
the codebase better structured as it is now easier to find the useful methods without having to sift
through the others, as some print methods were seemingly randomly placed throughout the files. The changes
made in this refactoring are very independent of the rest of the codebase, but they could still suggest
further refactorings are possible, as there could be some other methods within the code that are also
leftovers from debugging. Since this refactoring was relatively small, it was done within the main branch.
My fourth refactoring surrounded the Penalty class, specifically because it was left as a data class after
the refactoring that created it. I extracted and moved methods from the Schedule class’s eval methods that
help calculate penalty values to the Penalty class. These methods include slotMinPen(Slot, HashSet<Event>),
notPairedPen(Slot, Slot), and sectionPen(Slot, Event, Slot, Event). These methods were extracted and moved
respectively from the Schedule class’s evalMinFilled, evalPair and EvalSecDiff. To update the rest of the
codebase for this change, I changed the Schedule eval methods to call these new methods from the Schedule’s
Instance object. Since the unit tests already covered all methods involved, no changes to them were made.
This change was made in commit 815a19de8bfe04da219597864583f700fdf0086a. I also made the data that was
previously being publicly accessed private, as the Schedule class now exclusively calls the new public methods
to calculate penalties. This change was made in commit 48707fba59ab66e0482b63522ec00e9ab03d732f. The code is
better structured after this refactoring since the Schedule class’s many eval methods are now relatively
shorter in length, and some functionality involving the calculation of penalties is no longer made externally
from the Penalty class. However, not all of the functionality for calculating these penalties could be moved
with one refactoring. Most of the calculation requires the knowledge of slot-event pairs within a Schedule
object, which can only be calculated by methods within the Schedule class. A large refactoring involving the
creation of a new object for slot-event pairs that can be passed as a parameter to the Penalty class’s methods
would help with this. Since this refactoring was relatively small, it was made within the main branch.
My fifth refactoring was made in the GameSlot and PracticeSlot classes, specifically their overlaps() methods.
The problem I noticed was a long chain of if/else statements, all leading to setting two boolean variables,
dayOverlaps and timeOverlaps to true or false. I removed the conditional statements and replaced them in each
class with two lines, each setting the two boolean variables to what the conditional chains defined them as.
This change was made in commit Since all code within the GameSlot and PracticeSlot classes was already covered
in previously-made unit tests, these tests were not changed as part of this refactoring. The code is better
structured now since the simplification of the setting of these variables makes the code more organized, so the
variables’ purpose is more easily understood at first glance. Due to the small scale of this refactoring, all
changes were made within the main branch, and the result does not suggest any further refactorings. However,
other similar conditional chains could exist elsewhere in the codebase and require similar refactorings.
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment