Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorporate minimum value into 99 Bottles verse #2

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

trishrempel
Copy link
Owner

@trishrempel trishrempel commented Jan 15, 2023

Introduce the requirement that when CountdownSong specifies a min: parameter, the final BottleVerse should:

  • Display Go to the store and buy some more for action
  • Retain what quantity and container typically return for the minimum bottle number
  • Display the max bottle number for successor

For example, the last verse of this song:

CountdownSong.new(
  verse_template: BottleVerse,
  max: 6,
  min: 1
).song

Should be:

1 bottle of beer on the wall, 1 bottle of beer.
Go to the store and buy some more, 1 six-pack of beer on the wall.

Instead of:

1 bottle of beer on the wall, 1 bottle of beer.
Take it down and pass it around, no more bottles of beer on the wall.

The commits in this PR introduce an integration test with the new requirement, and then make the incremental changes necessary to make the test pass, according to the principles described in 99 Bottles of OOP by Sandi Metz.

First round of changes (till this commit)

  • Passed the min: parameter to BottleNumber.for and BottleVerse
  • Created a BottleNumberMin subclass of BottleNumber. It uses the BottleNumber.for factory to construct a BottleNumber with a min: value of nil and uses it as a proxy for quantity and container
  • Updated BottleNumber.for to return early with a BottleNumberMin instance if number equals min:
  • Updated CountdownSong#verse to pass a min: value to verse_template.lyrics
  • Removed the now unnecessary duplicate methods #action and #successor from BottleNumber0

Problem

I wasn't fond of this line in BottleNumberMin#new:

@bottle_number = BottleNumber.for(number, max: max, min: nil)

Here, it uses the BottleNumber.for factory to construct a BottleNumber for number so that it can be used as a proxy for quantity and container. In order to bypass constructing a BottleNumberMin once again, it explicitly passes min: as nil.

Doing this presumes an intimate knowledge of the inner workings of BottleNumber.for and introduces tight coupling and fragility to the code, according to the principles taught in 99 Bottles of OOP.

Second round of changes (from this commit till this commit)

I wanted to find a way to make BottleNumberMin less responsible and knowledgeable of the BottleNumber.for factory. To do this, I needed to shift the responsibility to BottleNumber.for.

  • Updated BottleNumberMin#new to accept a bottle_number parameter, to act as the proxy for #quantity and #container
  • Updated BottleNumber.for to first construct a BottleNumber for the number parameter. If number is equal to min, return a new BottleNumberMin, passing the usual properties and the BottleNumber. Otherwise, return the BottleNumber.

Follow the commits to see how I made the changes incrementally while keeping tests green.

Problem

BottleNumber.for now has a newline between two blocks of code, which usually represents a method doing too many things. The first block constructs a BottleNumber according to number. The second block decides whether to construct a BottleNumberMin with the already created BottleNumber as a parameter, or to return the existing BottleNumber.

To me, these are still clearly the responsibility of the BottleNumber factory. It might be possible to move these two blocks to their own private methods. However, I don't think this would significantly improve clarity and readability, and because BottleNumber.for is a class method, I think introducing more class methods that are private would be even more clunky and confusing.

@trishrempel trishrempel marked this pull request as ready for review January 16, 2023 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant