7
\$\begingroup\$

So in my free time, I made a discord bot to play around with and learn a bit of python. I'm mainly a java developer. The bot has a few commands that allow it to find and output recipe for cocktails and a few other things revolving around cocktails.

In the background, there are 2 JSON files. My bot mainly reads and filters these JSON files and returns what it finds. In java, I am used to streams which I don't have in python so I just had to make do with what I know but I get a feeling that some things can definitely be done cleaner/shorter/better/ and I'm not sure everything follows python standards so I would be grateful for any feedback.


 [ !!             cocktails can be exchanged with -c, ingredients with -i                !! ]
 [ <KW> find cocktails <search criteria1> <search criteria2> ...             Find cocktail. ]
 [ <KW> find ingredients <search criteria1> <search criteria2> ...         Find ingredient. ]
 [ <KW> cocktails                                                       List all cocktails. ]
 [ <KW> ingredients                                                   List all ingredients. ]
 [ <KW> categories                                            List all cocktail categories. ]
 [ <KW> list <category>                                      List all cocktails by category ]
 [ <KW> count cocktails                                                 How many cocktails? ]     
 [ <KW> count ingredients                                             How many ingredients? ]    
 [ <KW> with <ingredient> <ingredient2> ...                      Cocktails with ingredient. ]     
 [ <KW> recipe <cocktail>                                           How to make a cocktail. ]

The possible commands are as above <KW> is the trigger word for the bot that a different component handles. bartender.py receives the content with this cut off.


bartender.py

import json

class Bartender:
    with open("recipes.json") as recipes_file:
        recipes = json.load(recipes_file)

    with open("ingredients.json") as ingredients_file:
        ingredients = json.load(ingredients_file)

    default = "This command does not exist or you mistyped something"
    error = "There was a problem with processing the command"

    def handle(self, message):
        command_prefix = message.content.strip().lower()
        answer = self.default
        if self.starts_with_ingredients_prefix(command_prefix):
            answer = self.get_all_ingredients()
        elif self.starts_with_cocktails_prefix(command_prefix):
            answer = self.get_all_cocktails()
        elif command_prefix == "categories":
            answer = self.get_all_categories()
        elif command_prefix.startswith("count"):
            answer = self.get_count(command_prefix.removeprefix("count").strip())
        elif command_prefix.startswith("recipe"):
            answer = self.get_recipe(command_prefix.removeprefix("recipe").strip())
        elif command_prefix.startswith("list"):
            answer = self.get_cocktails_by_category(command_prefix.removeprefix("list").strip())
        elif command_prefix.startswith("find"):
            answer = self.find(command_prefix.removeprefix("find").strip())
        elif command_prefix.startswith("with"):
            answer = self.get_cocktails_with(command_prefix.removeprefix("with").strip())
        return answer

    def get_all_ingredients(self):
        """Returns a string containing all of the ingredients the bot knows."""
        answer = ""
        for key, value in self.ingredients.items():
            answer += f"{key} ({value.get('abv')}%)\n"
        return answer

    def get_all_cocktails(self):
        """Returns a string containing the names of all of the cocktails the bot knows."""
        answer = ""
        for cocktail in self.recipes:
            answer += f"{cocktail.get('name')}\n"
        return answer

    def get_all_categories(self):
        """Returns a string containing all the cocktail categories the bot knows."""
        answer = ""
        categories_list = []
        for cocktail in self.recipes:
            categories_list.append(cocktail.get("category"))
        # Remove duplicates
        categories_list = list(set(categories_list))
        categories_list.sort(key=str)
        for category in categories_list:
            answer += f"{category}\n"
        return answer

    def get_count(self, param):
        """Returns the amount of ingredients or cocktails the bot knows."""
        answer = self.error
        if self.starts_with_ingredients_prefix(param):
            answer = len(self.ingredients)
        elif self.starts_with_cocktails_prefix(param):
            answer = len(self.recipes)

        return answer

    def get_recipe(self, param):
        """Returns the full recipe for the passed cocktail name."""
        answer = f"There is no recipe for a cocktail called {param}. To see all cocktails with a recipe " \
                 f"type '$bt cocktails'"
        for cocktail in self.recipes:
            if param == cocktail.get("name").lower():
                formatted_ingredients = self.get_formatted_ingredients(cocktail.get("ingredients"))
                garnish = self.get_garnisch(cocktail)
                return f"__**{cocktail.get('name')}**__\n" \
                       f"**Ingriedients:**\n" \
                       f"{formatted_ingredients}" \
                       f"{garnish}" \
                       f"**Preparation:**\n" \
                       f"{cocktail.get('preparation')} \n"
        return answer

    def get_formatted_ingredients(self, ingredients):
        """Returns a string of ingredients formatted as list for the cocktails including the special ones if it has
        any."""
        formatted_ingredients = ""
        special_ingredients = ""
        for ingredient in ingredients:
            if ingredient.get("special") is not None:
                special_ingredients += f" - {ingredient.get('special')}\n"
            else:
                formatted_ingredients += f" - {ingredient.get('amount')} {ingredient.get('unit')} {ingredient.get('ingredient')} "
                if ingredient.get("label") is not None:
                    f"({ingredient.get('label')})"
                formatted_ingredients += "\n"

        return formatted_ingredients + special_ingredients

    def get_garnisch(self, cocktail):
        """Returns the garnish for the cocktail if it has one."""
        if cocktail.get("garnish") is not None:
            return f"**Garnish:**\n" \
                   f" - {cocktail.get('garnish')} \n"
        else:
            return ""

    def get_cocktails_by_category(self, category):
        """Returns all cocktails in the given category."""
        answer = ""
        for cocktail in self.recipes:
            if category == str(cocktail.get("category")).lower():
                answer += f"{cocktail.get('name')}\n"

        return answer if len(answer) > 0 else f"There is no category called {category} or it contains no cocktails"

    def starts_with_cocktails_prefix(self, param):
        """Returns true if passed string starts with the cocktails prefix (-c or cocktails)."""
        return param.startswith("-c") or param.startswith("cocktails")

    def remove_cocktails_prefix(self, param):
        """Returns a string with the cocktails prefix (-c or cocktails) removed. If the string does not start with
        the cocktails prefix it will return the original string."""
        if param.startswith("-c"):
            param = param.removeprefix("-c")
        elif param.startswith("cocktails"):
            param = param.removeprefix("cocktails")
        return param

    def starts_with_ingredients_prefix(self, param):
        """Returns true if passed string starts with the ingredient prefix (-i or ingredients)."""
        return param.startswith("-i") or param.startswith("ingredients")

    def remove_ingredients_prefix(self, param):
        """Returns a string with the ingredient prefix (-i or ingredients) removed. If the string does not start with
        the ingredients prefix it will return the original string."""
        if param.startswith("-i"):
            param = param.removeprefix("-i")
        elif param.startswith("ingredients"):
            param = param.removeprefix("ingredients")
        return param

    def find(self, param):
        """Returns all ingredients or cocktails containing the criteria in the parameter separated by commas."""
        answer = ""
        if self.starts_with_cocktails_prefix(param):
            param = self.remove_cocktails_prefix(param)
            for criteria in param.strip().split():
                answer += f"**Criteria: {criteria}**\n"
                answer += self.get_cocktails_containing(criteria)
        elif self.starts_with_ingredients_prefix(param):
            param = self.remove_ingredients_prefix(param)
            for criteria in param.strip().split():
                answer += f"**Criteria: {criteria}**\n"
                answer += self.get_ingredients_containing(criteria)

        return answer if len(answer) > 0 else "Nothing was found matching your criteria"

    def get_cocktails_containing(self, criteria):
        """Returns all cocktails containing the criteria in its name."""
        answer = ""
        for cocktail in self.recipes:
            if criteria in str(cocktail.get("name")).lower():
                answer += f"{cocktail.get('name')}\n"

        return answer if len(answer) > 0 else "Nothing was found matching your criteria"

    def get_ingredients_containing(self, criteria):
        """Returns all ingredients containing the criteria in its name."""
        answer = ""
        for ingredient in self.ingredients.keys():
            if criteria in ingredient.lower():
                answer += f"{ingredient}\n"

        return answer if len(answer) > 0 else "Nothing was found matching your criteria"

    def get_cocktails_with(self, param):
        """Returns all cocktails containing the searched for ingredients in the parameter separated by commas."""
        answer = ""
        for ingredient in param.strip().split(","):
            for cocktail in self.recipes:
                cocktail_ingredients = cocktail.get("ingredients")
                answer += self.does_cocktail_contain(cocktail, cocktail_ingredients, ingredient.strip())

        return answer if len(answer) > 0 else "Nothing was found matching your criteria"

    def does_cocktail_contain(self, cocktail, cocktail_ingredients, ingredient):
        """Returns the name of the cocktail if the cocktail contains the searched for ingredient."""
        for cocktail_ingredient in cocktail_ingredients:
            if cocktail_ingredient.get("ingredient") is not None and ingredient in cocktail_ingredient.get(
                    "ingredient").lower():
                return f"{cocktail.get('name')}\n"

        return ""


recipes.json

[
  { "name": "Vesper",
    "glass": "martini",
    "category": "Before Dinner Cocktail",
    "ingredients": [
      { "unit": "cl",
        "amount": 6,
        "ingredient": "Gin" },
      { "unit": "cl",
        "amount": 1.5,
        "ingredient": "Vodka" },
      { "unit": "cl",
        "amount": 0.75,
        "ingredient": "Lillet Blonde" },
      { "special": "3 dashes Strawberry syrup" }
    ],
    "garnish": "Lemon twist",
    "preparation": "Shake and strain into a chilled cocktail glass." 
  },
  ...
]

Not all cocktails have the glass attribute and a cocktail can have 0 to n "special" ingredients.


ingredients.json

{
  "Absinthe": {
    "abv": 40,
    "taste": null
  },
  "Aperol": {
    "abv": 11,
    "taste": "bitter"
  },
  ...
}

\$\endgroup\$
1
  • 1
    \$\begingroup\$ "I am used to streams which I don't have in python" -- when you learn a bit more of python, you'll be delighted to see that Javas Streams are just a kludge to achieve a subset of the things you can achieve with pythons list/set/dict comprehensions and generators. In Python, you just don't have a buzzword for it, because all of that is already baked into the basic language instead of being an "add-on" via the standard library. These functional concepts can be taken even further, but you'll need a pure functional language like Haskell for that. \$\endgroup\$ – orithena Mar 31 at 13:36
4
\$\begingroup\$

get_all_ingredients

List comprehensions are very pythonic and often times faster (especially for operations, that aren't computationally expensive), so instead of

answer = ""
for key, value in self.ingredients.items():
    answer += f"{key} ({value.get('abv')}%)\n"

you should use

answer = "".join(f"{key} ({value.get('abv')}%)\n" for key, value in self.ingredients.items())
return answer

or as a one-liner

return "".join(f"{key} ({value.get('abv')}%)\n" for key, value in self.ingredients.items())

str.join(...) joins each element of an iterable by a string separator (the string on which it is called) and returns the concatenated string.

What we're using here is actually not a list comprehension, but a generator expression. If you're interested you can read more about Generator Expressions vs List Comprehensions.

The same is also applicable to get_all_cocktails.


get_all_categories

As seen before we can replace

categories_list = []
for cocktail in self.recipes:
    categories_list.append(cocktail.get("category"))

by a list comprehension. But since we actually want a sorted list of distinct values we can use a set comprehension or use set() and a generator expression.

{cocktail.get("category") for cocktail in self.recipes}
# or
set(cocktail.get("category") for cocktail in self.recipes)

Now we wrap this with sorted() which returns a sorted list and we get categories_list concisely. Providing str as the key function should not be necessary since the values are already strings.

categories_list = sorted(set(cocktail.get("category") for cocktail in self.recipes))

Using str.join() again brings the entire method down to 2 lines:

categories_list = sorted(set(cocktail.get("category") for cocktail in self.recipes))
return "".join(f"{category}\n" for category in categories_list)

Note that we don't need to assign an empty string as a default value since "".join(...) will return an empty string if there are no elements in the iterable.


get_formatted_ingredients

You will sometimes be able to replace

if ingredient.get("special") is not None:

by

if ingredient.get("special"):

This however depends on the use case, since the expressions are not equivalent. The second one will fail for all falsey values, not just None (e.g. an empty string in this case, which you also might want to skip). If you're interested, you can read more about Truthy and Falsy Values in Python.

Using the walrus operator we can make this more concise, and remove the redundant get:

if special_ingredient := ingredient.get("special") is not None:
    special_ingredients += f" - {special_ingredient}\n"

I don't think the following code does anything at all, since it doesn't do anything with the string:

if ingredient.get("label") is not None:
    f"({ingredient.get('label')})"

These are just some things I noticed. You will find these patterns in different places throughout your code. This is of course not a complete list of possible improvements, just the ones I found on my first read through your code.

\$\endgroup\$
9
  • 2
    \$\begingroup\$ list comprehensions are [...] generally more efficient - That depends on a lot of factors, and won't always be the case; but here efficiency is not really an issue \$\endgroup\$ – Reinderien Mar 30 at 22:51
  • \$\begingroup\$ You may also want to consider using a set literal {} and not a named set() constructor. \$\endgroup\$ – Reinderien Mar 30 at 22:52
  • 1
    \$\begingroup\$ Your recommendation to try .removeprefix("-c").removeprefix("cocktails") is buggy. What if a user passes -ccocktails? \$\endgroup\$ – Reinderien Mar 30 at 22:56
  • 1
    \$\begingroup\$ Thanks for your comments, I removed or corrected the incorrect claims. The set literal was already included in my answer, but I often find the named constructor more readable, especially in conjunction with sorted() etc.. I haven't yet come across a use case where loops clearly outperform list comprehensions, I'd be interested in learning more about the performance differences between the two if you know of any resources. \$\endgroup\$ – riskypenguin Mar 30 at 23:11
  • 1
    \$\begingroup\$ Thank you very much for the great feedback looks like I have a lot to do :D. Do you have any ideas on making the handle method better. I'm not a fan of the else if however, I started with a switcher but after a certain point, it wasn't possible anymore. \$\endgroup\$ – Lucifer Uchiha Mar 31 at 6:24
3
\$\begingroup\$

Instance-ness

Your file reading is done in class static scope. It should be moved to class instance scope.

Type hints

Add some PEP484 type hints to your function signatures.

Type soundness and dictionary soup

Once you've read in your data from JSON, you should not leave it as dictionaries - make actual classes so that your data have stronger correctness guarantees and static analysis tools can actually say something about your code.

Another benefit of this approach is that your new classes for recipes and ingredients can have methods dealing with data specific to that class.

Indexing

You haven't built up any index dictionaries for faster, simpler lookup based on search terms; you should.

Line continuation

For long string, prefer to put them in parenthesized expressions rather than adding backslashes.

Bug: label

This statement:

f"({ingredient.get('label')})"

does nothing, so you'll never see your label.

Spelling

garnisch -> garnish

Ingriedients -> Ingredients

Documentation lies

  """Returns all ingredients or cocktails containing the criteria in the parameter separated by commas."""

is not, in fact, what's happening. Your split() splits on whitespace.

Statics

Any method that doesn't actually touch the self reference doesn't need to be an instance method and can benefit from being a static, or sometimes a classmethod if (in simple terms) it references other statics or the constructor. In reality @classmethods can reference any attribute on the class object which is a touch more detail than I suspect you need right now.

Benefits to proper use of static methods include easier testability due to fewer internal dependencies, and more accurate representation of "what's actually happening" to other developers reading your code.

It's also worth mentioning (due to your comment) that public/private and instance/static are very different mechanisms. public/private, in other languages, controls visibility from the outside of the class, whereas instance/static controls whether the method is visible on an instance or on the class type itself. Python does not have a strong concept of public/private at all, aside from the leading-underscore-implying-private and double-underscore-implying-name-mangling conventions; neither is called for here I think.

Suggested

There are many ways to implement some of the above, particularly conversion to classes. Presented below is one way.

import json
from collections import defaultdict
from dataclasses import dataclass
from itertools import chain
from typing import Optional, Iterable, Tuple, Dict, Any, List


@dataclass
class FakeMessage:
    content: str


@dataclass
class Ingredient:
    name: str
    abv: float
    taste: Optional[str]

    @classmethod
    def from_json(cls, fname: str = 'ingredients.json') -> Iterable['Ingredient']:
        with open(fname) as ingredients_file:
            data = json.load(ingredients_file)

        for name, attributes in data.items():
            yield cls(name=name, **attributes)

    def __str__(self):
        return f'{self.name} ({self.abv}%)'


@dataclass
class RecipeIngredient:
    unit: str
    amount: float
    ingredient: Ingredient
    label: Optional[str] = None

    @classmethod
    def from_dicts(
        cls,
        dicts: Iterable[Dict[str, Any]],
        all_ingredients: Dict[str, Ingredient],
    ) -> Iterable['RecipeIngredient']:
        for data in dicts:
            if 'special' not in data:
                name = data.pop('ingredient')
                yield cls(**data, ingredient=all_ingredients[name.lower()])

    def __str__(self):
        desc = f'{self.amount} {self.unit} {self.ingredient}'
        if self.label is not None:
            desc += f' {self.label}'
        return desc


@dataclass
class Recipe:
    name: str
    glass: str
    category: str
    preparation: str
    ingredients: Tuple[RecipeIngredient, ...]
    special_ingredients: Tuple[str, ...]
    garnish: Optional[str] = None

    @classmethod
    def from_json(
        cls,
        all_ingredients: Dict[str, Ingredient],
        fname: str = 'recipes.json',
    ) -> Iterable['Recipe']:
        with open(fname) as recipes_file:
            data = json.load(recipes_file)

        for recipe in data:
            ingredient_data = recipe.pop('ingredients')
            specials = tuple(
                ingredient['special']
                for ingredient in ingredient_data
                if 'special' in ingredient
            )
            yield cls(
                **recipe,
                ingredients=tuple(
                    RecipeIngredient.from_dicts(ingredient_data, all_ingredients)
                ),
                special_ingredients=specials,
            )

    @property
    def formatted_ingredients(self) -> str:
        """Returns a string of ingredients formatted as list for the cocktails including the special ones if it has
        any."""
        ingredients = chain(self.ingredients, self.special_ingredients)
        return '\n'.join(
            f' - {ingredient}'
            for ingredient in ingredients
        )

    @property
    def as_markdown(self) -> str:
        desc = (
            f"__**{self.name}**__\n"
            f"**Ingredients:**\n"
            f"{self.formatted_ingredients}\n"
        )

        if self.garnish is not None:
            desc += f' - {self.garnish}\n'

        desc += (
            f"**Preparation:**\n"
            f"{self.preparation}\n"
        )
        return desc


class Bartender:
    ERROR = "There was a problem with processing the command"

    def __init__(self):
        self.ingredients: Dict[str, Ingredient] = {
            i.name.lower(): i for i in Ingredient.from_json()
        }
        self.recipes: Dict[str, Recipe] = {
            r.name.lower(): r for r in Recipe.from_json(self.ingredients)
        }

        self.categories: Dict[str, List[Recipe]] = defaultdict(list)
        for recipe in self.recipes.values():
            self.categories[recipe.category.lower()].append(recipe)

        self.by_ingredient: Dict[str, List[Recipe]] = defaultdict(list)
        for recipe in self.recipes.values():
            for ingredient in recipe.ingredients:
                self.by_ingredient[ingredient.ingredient.name.lower()].append(recipe)

    def handle(self, message: FakeMessage) -> str:
        command_prefix = message.content.strip().lower()

        if self.starts_with_ingredients_prefix(command_prefix):
            return self.get_all_ingredients()
        if self.starts_with_cocktails_prefix(command_prefix):
            return self.get_all_cocktails()
        if command_prefix == "categories":
            return self.get_all_categories()
        if command_prefix.startswith("count"):
            return self.get_count(command_prefix.removeprefix("count").strip())
        if command_prefix.startswith("recipe"):
            return self.get_recipe(command_prefix.removeprefix("recipe").strip())
        if command_prefix.startswith("list"):
            return self.get_cocktails_by_category(command_prefix.removeprefix("list").strip())
        if command_prefix.startswith("find"):
            return self.find(command_prefix.removeprefix("find").strip())
        if command_prefix.startswith("with"):
            return self.get_cocktails_with(command_prefix.removeprefix("with").strip())

        return "This command does not exist or you mistyped something"

    def get_all_ingredients(self) -> str:
        """Returns a string containing all of the ingredients the bot knows."""
        return '\n'.join(str(i) for i in self.ingredients.values())

    def get_all_cocktails(self) -> str:
        """Returns a string containing the names of all of the cocktails the bot knows."""
        return '\n'.join(r.name for r in self.recipes.values())

    def get_all_categories(self) -> str:
        """Returns a string containing all the cocktail categories the bot knows."""
        categories = sorted({
            recipe.category
            for recipe in self.recipes.values()
        })
        return '\n'.join(categories)

    def get_count(self, param: str) -> str:
        """Returns the amount of ingredients or cocktails the bot knows."""
        if self.starts_with_ingredients_prefix(param):
            sequence = self.ingredients
        elif self.starts_with_cocktails_prefix(param):
            sequence = self.recipes
        else:
            return self.ERROR

        return str(len(sequence))

    def get_recipe(self, param: str) -> str:
        """Returns the full recipe for the passed cocktail name."""
        recipe = self.recipes.get(param)
        if recipe is None:
            return (
                f"There is no recipe for a cocktail called {param}. To see all "
                f"cocktails with a recipe type '$bt cocktails'"
            )
        return recipe.as_markdown

    def get_cocktails_by_category(self, category: str) -> str:
        """Returns all cocktails in the given category."""
        recipes = self.categories.get(category)
        if not recipes:
            return f"There is no category called {category} or it contains no cocktails"

        return '\n'.join(r.name for r in recipes)

    @staticmethod
    def starts_with_cocktails_prefix(param: str) -> bool:
        """Returns true if passed string starts with the cocktails prefix (-c or cocktails)."""
        return param.startswith("-c") or param.startswith("cocktails")

    @staticmethod
    def remove_cocktails_prefix(param: str) -> str:
        """Returns a string with the cocktails prefix (-c or cocktails) removed. If the string does not start with
        the cocktails prefix it will return the original string."""
        if param.startswith("-c"):
            return param.removeprefix("-c")
        if param.startswith("cocktails"):
            return param.removeprefix("cocktails")
        return param

    @staticmethod
    def starts_with_ingredients_prefix(param: str) -> bool:
        """Returns true if passed string starts with the ingredient prefix (-i or ingredients)."""
        return param.startswith("-i") or param.startswith("ingredients")

    @staticmethod
    def remove_ingredients_prefix(param: str) -> str:
        """Returns a string with the ingredient prefix (-i or ingredients) removed. If the string does not start with
        the ingredients prefix it will return the original string."""
        if param.startswith("-i"):
            return param.removeprefix("-i")
        if param.startswith("ingredients"):
            return param.removeprefix("ingredients")
        return param

    def find(self, param: str) -> str:
        """Returns all ingredients or cocktails containing the criteria in the parameter separated by commas."""
        answer = ""
        if self.starts_with_cocktails_prefix(param):
            param = self.remove_cocktails_prefix(param)
            for criteria in param.strip().split():
                answer += f"**Criteria: {criteria}**\n"
                answer += self.get_cocktails_containing(criteria)
            return answer

        if self.starts_with_ingredients_prefix(param):
            param = self.remove_ingredients_prefix(param)
            for criteria in param.strip().split():
                answer += f"**Criteria: {criteria}**\n"
                answer += self.get_ingredients_containing(criteria)
            return answer

        return self.ERROR

    def get_cocktails_containing(self, criteria: str) -> str:
        """Returns all cocktails containing the criteria in its name."""
        answer = ""
        for name, recipe in self.recipes.items():
            if criteria in name:
                answer += f"{recipe.name}\n"

        if answer:
            return answer
        return "Nothing was found matching your criteria"

    def get_ingredients_containing(self, criteria: str) -> str:
        """Returns all ingredients containing the criteria in its name."""
        answer = ""
        for name, ingredient in self.ingredients.items():
            if criteria in name:
                answer += f"{ingredient.name}\n"

        if answer:
            return answer
        return "Nothing was found matching your criteria"

    def get_cocktails_with(self, param: str) -> str:
        """Returns all cocktails containing the searched for ingredients in the parameter separated by commas."""
        answer = '\n'.join(
            recipe.name
            for ingredient in param.strip().split(",")
            for recipe in self.by_ingredient.get(ingredient, ())
        )

        if answer:
            return answer
        return "Nothing was found matching your criteria"


def test():
    bar = Bartender()
    while True:
        try:
            cmd = input('> ')
        except KeyboardInterrupt:
            break

        print(bar.handle(FakeMessage(cmd)))
        print()

test()
\$\endgroup\$
7
  • \$\begingroup\$ Thank you very much for the great feedback :D Do you have any ideas on making the handle method better? I'm not a fan of the big elif. hHowever, I started with a switcher but after a certain point, it wasn't possible anymore. \$\endgroup\$ – Lucifer Uchiha Mar 31 at 6:26
  • \$\begingroup\$ I have 2 questions after further looking at your suggestion. What is the FakeMessage for? I can't really wrap my head around it since it only gets used in the handle. And the other question is why some methods are static? In java, I would normally make all of the methods private apart from the handle but that's not possible as far as I \$\endgroup\$ – Lucifer Uchiha Mar 31 at 7:20
  • 2
    \$\begingroup\$ This is a very opiniated answer and does not reflect broad consesus. It's not bad per se, just be wary following this anwser. \$\endgroup\$ – Hakaishin Mar 31 at 7:32
  • \$\begingroup\$ @Hakaishin It's "opiniated" for a reason - I'm happy to share more justification for any points that you think need them. \$\endgroup\$ – Reinderien Mar 31 at 13:02
  • \$\begingroup\$ @LuciferUchiha FakeMessage is just what I used to mock out actual Discord, so that the test doesn't need it. In the actual code, there will be a type from the Discord library that you'll want to reference. \$\endgroup\$ – Reinderien Mar 31 at 13:03
1
\$\begingroup\$

You could replace some for loops with list comprehensions. I think list comprehensions are generally preferred and often faster. I've also:

  • added a default value to the get command in case your JSON is malformed or missing the ABV for some reason, and
  • replaced the \n on the end of each line with the join method.
def get_all_ingredients(self):
    """Returns a string containing all of the ingredients the bot knows."""
    answer = ""
    for key, value in self.ingredients.items():
        answer += f"{key} ({value.get('abv')}%)\n"
    return answer

def get_all_ingredients(self):
    answers = [f'{key} ({value.get("drink_info", "-")}%)' for key, value in self.ingredients.items()]
    return '\n'.join(answers)
\$\endgroup\$
3
  • 1
    \$\begingroup\$ '\n'.join(answers) is different than OP's implementation, since it does not include a trailing newline. Might or might not be an issue depending on the use case. \$\endgroup\$ – riskypenguin Mar 30 at 23:16
  • \$\begingroup\$ Welcome to CodeReview@SE. It's entirely OK for an answer to raise but one valid point. (In fact, I think the tendency towards comprehensive reviews unfavorably increases the threshold to answer.) \$\endgroup\$ – greybeard Mar 31 at 10:24
  • \$\begingroup\$ (Then again, this adds little to Reinderien's take on get_all_ingredients(), while dropping the docstring. And it seems to repeat code from the question, which is unusual.) \$\endgroup\$ – greybeard Mar 31 at 10:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.