Refactor main#update_quality (with mutant) #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* This will execute the tests for each commit.
* This allows the tests to "pass" as long as the tests marked as pending continue to fail. This will allow me to refactor the existing code to be simple enough that adding support for the conjured items will be easier. Once support is added these tests will begin to fail, and the tests will no longer be marked as pending.
* Move main#update_quality to GildedRose.update_quality so that it is no longer in the main namespace. This will make it easier for tooling to select it in the future.
* Configure CI so new methods are checked with mutant
* Use the Replace Method with Method Object refactoring:
https://refactoring.guru/replace-method-with-method-object
* A temporary ItemUpdater#item alias method is added to allow the
code to remain mostly unchanged from the original. A future
commit will remove the alias and allow the code to delegate
implicitly via self rather than item.
* This forces the code to use explicit self instead of the item alias to access the delegated object.
* Add ItemUpdater#quality= and use Comparable#clamp to limit the upper and lower bounds on the quality. * Remove all refundant upper and lower bounds checks in ItemUpdater#call.
* It is difficult to reason about negated conditionals, so when there is an if/else branch with a negative conditional I flipped the branches around so that it is easier to read and understand.
* If an else contains another if/else branch, then it can be collapsed and moved up to the parent branch.
* A future refactoring will transform this into an if/else condition, but I wanted to minimize the diff so that it was easier to understand in a smaller change.
* The nested conditional is not necessary, and it is cleaner to move it up to the parent conditional. Note that this does cause some minor duplication where the quality is incremented in the aged brie and backspace pass branches.
* When the quality is being incremented twice in the block it is a little more difficult to understand than a simple if/else/else block where it is incremented *exactly* the correct number of times based on the sell_in value.
* The code duplicates the quality mutator call when the only detail that differs is how much the value should be incremented.
* The quality reset used a redundant expression that can just be simplified to 0.
* This groups the quality changing code closer together so that it will be more clear when they are collapsed into the same logic, in an upcoming commit.
* The use of -quality effectively resets the quality back to 0 after expiry, since a backstage pass is worthless after the event has finished.
* It can be more difficult to follow logic when values are mutated more than once in a given expression.
* This changes the logic so that self.quality is always incremented with += so that the value amount can be extracted into a separate method in a future commit.
* Change tests to no longer be pending
* Use the Extract Method refactoring:
https://refactoring.guru/extract-method
* TODO: These fixes should be moved to an earlier commit
* Change main#update_quality to match the updater by the item name
* This allows the removal of the conditional logic in ItemUpdater#call
* This simplifies ItemUpdater#change_quality
* Refactor ItemUpdater#value_change to be simpler
* This simplifies ItemUpdater#value_change
* This simplifies ItemUpdater#value_change
dkubb
force-pushed
the
dkubb/solution2
branch
from
6621491
to
88ce3f5
Compare
dkubb
commented
Sep 1, 2020
dkubb
force-pushed
the
dkubb/solution2
branch
4 times, most recently
from
681a0ed
to
561270a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.