Ingredient command line options#6
Conversation
| case | ||
| when ingredients.none? |
There was a problem hiding this comment.
Since this is an empty case statement I think it's equivalent toif/elsif?
| private | ||
|
|
||
| def recipe_info | ||
| def recipe_info(recipe) |
There was a problem hiding this comment.
Almost certainly overkill but all these methods take recipe as an argument, meaning there could be an object here e.g.:
class HTMLRecipe
def initialize(recipe); @recipe = recipe; end
def info; ... end
def ingredients; ... end
def instructions; ... end
endso then
Line 2 in 8b3c9fb
def present(recipe)
<<~STR
#{recipe.info}
#{recipe.ingredients}
...
STR
endand we wouldn't need e.g.
Line 12 in 8b3c9fb
That feels definitely overkill - it's an extra object that returns HTML given a Recipe, but it feels like it crosses too many layers of abstraction at once. Better to, as you've done here, use methods on the domain object to build up the HTML, within the same class.
It might be interesting to consider templating, similar to Rails views. That may clean up the output << "myhtml" stuff, since it could use the templating loop functionality.
Another comment more generally is about inheriting from Presenter. Implementing present in the parent class is quite restrictive. A way to think about it is to ask what do HtmlPresenter and PlainTextPresenter share in behaviour? The fact that they can stringify Recipes i.e.
present :: Recipe -> String
is possibly all. That'd mean that having an implementation of present in the parent class is too restrictive - if we want to wrap the HtmlPresenters output in e.g. a <body> tag we'd have to override present, suggesting it wasn't shared behaviour between Presenters.
| all_recipes.select do |recipe| | ||
| ingredients.all? { |ingredient| recipe.contains?(ingredient) } | ||
| end |
There was a problem hiding this comment.
Nice 👍 I like the split between the repository and using methods on the domain object.
| ingredients = ARGV | ||
| cli = Cli.new | ||
|
|
||
| case |
There was a problem hiding this comment.
Possibly an opportunity for pattern matching (case in) on ingredients.
There was a problem hiding this comment.
Maybe:
case ingredients
in []
puts cli.sample
in [a, a, a, a]
cli.select_recipes(ingredients: ingredients)
else
...
| selected_recipe = RecipeSelector.new(recipes: matching_recipes).select | ||
| puts presenter.present(recipe: selected_recipe) |
There was a problem hiding this comment.
Possibly rename to TerminalRecipeSelector. Could also use as an instance variable to hide which one we use e.g.:
selected_recipe = recipe_selector.select_one_from(matching_recipes)
| recipe_data["ingredient_entries"].map do |ingredient_entry| | ||
| ingredient_entry["ingredient"] | ||
| end |
There was a problem hiding this comment.
Could be interesting to return an Ingredient object here. We could do things like group ingredients together:
class RecipeBook # or whatever name
def ingredients
[
Ingredient.new("lime"),
SynonymGroup.new([Ingredient.new("bacon"), Ingredient.new("pancetta")]),
Ingredient.new("chicken stock"),
]
end
endI imagine then given an input ingredient we'd iterate through this list until we find the first one that matches, then use that as our canonical ingredient object. We could do the same when creating a Recipe object.
e.g.
class Ingredient
def initialize(name)
@name = name
end
def match?(name)
@name =~ name # can do case insensitive matches here too
end
end
class SynonymGroup
def initialize(ingredients)
@ingredients = ingredients
end
def match?(name)
@ingredients.any? { |ingredient| ingredient.match?(name) }
end
end2 participants
Introduces TTY prompt to allow user to input up to 4 ingredients as command line options, then select from a list of recipes containing those ingredients.