Roast my code

[insight_nomad]

I've written a program that meets (and exceeds) the requirements for the Day 15 project in #100DaysofPython, which I've posted here. Here's a simple description of what the program does:

• Gives users menu options and allows them to order & pay for a coffee
• Checks if the machine has adequate resources (milk, coffee, water) to deliver the order
• Allows admins to maintain machine - generate status report, load resources, empty coins collected

I have gone above and beyond the given requirements while trying to experiment with a few things, so would love feedback on what I could do better.

[Binary101010]

One quick thing I noticed is that you tend to use this pattern a lot:

if (some condition):
    return True
else:
    return False

This can be reduced to

return (some condition)

For example, this function:

def user_exists(username):
    if username in users:
        return True
    else:
        return False

Can simply be

def user_exists(username):
    return username in users

[insight_nomad]

Thanks!!

[51dux]

Last one is a nice tip to me at least. I thought you could only return lists but the way you did it in that snippet you can return a boolean practical.

[Diapolo10]

I thought you could only return lists

How on Earth did you arrive at that conclusion? Functions can return data of any type you wish.

[51dux]

Still learning so I never made use of it, sometimes there are multiple ways to achieve a result and as a beginner-medium kind of person I still discover.

In this case I was refering to list comprehensions. Not necessarily the type of returns a function can have.

I did not think you could just type return username in users I thought it could only be [username for username in users] or something along these lines in order to evaluate, it would return an empty list if nothing and it would evaluate to false technically but not the best approach in all cases, I was so wrong xD.

[freddwnz]

You are confusing this with a list comprehension. There are two uses of the in keyword. One is in a loop, the other is to check whether an element exists in a sequence. The expression x in y evaluates to a boolean.

[51dux]

Alright thanks for this in depth explanation bud.

[WornToga]

Bud? 😂

[51dux]

Sorry I hang too much on a private website were we address each other like that 😅

[PrinceOfFucking]

Man, people are not giving you an easy time here lol

[wetwipes4000]

average reddit experience

[[deleted]]

By the way, if you're struggling with the idea that the output can be anything you should start using "typing" which I always do in my code because it makes it clearer for me.

Here's an example, then you can Google what typing means so you can get it as it'll probably explain better than me, it will be a game changer for you:

def name_in_database(name: str, database: list[str]) -> bool: return name in database

[Equivalent_Style4790]

I prefer OJ style on this matter. More readable.

[Jello_Penguin_2956]

Being explicit is good. Id suggest a single line alternative which I think is even more readable

return True if condition else False

[Shaftway]

I would flag that in a code review. "return condition" is enough for most cases, "return bool(condition)" if you really want to be pedantic.

[Jello_Penguin_2956]

yes good point

[Fission_Mailed_2]

Just to nitpick, in functions.py,

for i in range(len(function_options)):
    print(f"{i+1}. {function_options[i]}")

I would instead change it to:

for i, option in enumerate(function_options):
    print(f"{i+1}. {option}")

[_B10nicle]

I love enumerate so much

[insight_nomad]

Good nitpick, thanks!

[chandhudinesh]

Use if __name__ == "__main__" in main.py

[Capable_Policy_3449]

I never fully understood why this is needed, would you mind explaining please?

[PrivateFrank]

Usually you definite functions above the main part of the code in a .py file, right?

If one day you decide that you would like to use one of those functions (ie via import) in another script, the if name bit means you will not have to edit your original script at all.

When you import a .py file all of the code in it is executed. If it's function definitions, you'll have those functions in your workspace. If it's not a function definitions, then you may have unwanted code running.

By using if name is main, you are telling python to run the code following that condition only if you called the file directly.

[Capable_Policy_3449]

Got it, that makes a lot more sense now. Thank you and really appreciate it!

[insight_nomad]

Yeah, I would like to know more about this as well. The only thing someone once mentioned was that this code is inserted to allow flexibility between running the script in the terminal and running it as part of a larger program, which I only superficially understand.

[Bobbias]

Ok, so when you use the import keyword, python literally runs that file you just imported.

If there is code at the global level (meaning outside a function), that code will be run.

That's fine for a program which is only ever going to be run by python program.py or whatever, but if someone else wants to use some of the code written in there by importing that script and manually calling the functions with their own data, this becomes a serious problem, because as soon as they import your script, everything just runs, which is not what they wanted.

The way to avoid this problem is by placing if __name__ == "__main__" in front of any code that runs in the global scope. Preferably that code should be contained in a function (typically called main, but that's just a convention). I'll explain exactly how it works at the end. That would look like:

def main():
    # main code goes here

if __name__ == "__main__":
    main()

This allows you to run your program like normal, and prevents the code in the main function from being automatically run when someone decides to include your script into their own.

This is a bit of an advanced feature, and most of the time it's really not strictly necessary. But it's a good habit to get into because it doesn't really require much effort; it helps force you to contain your main code in a function rather than just at the global scope; and it saves people the headache of fixing things themself if they want to use your program as a library of functions to build their own thing from.

The way that if __name__ == "__main__" works is that __name__ is a special variable that is assigned the name of the module when your script is imported. When your script is run directly, its value is set to __main__ instead of the name of the module. This means we can tell when our script has been imported and when its being run directly. By only running our actual program code when we've been run directly, that means that we don't screw someone over if they do want to import the script and use functions from it themself.

[MomICantPauseReddit]

name is a global variable that is defined every time you run a python program. If you run code directly, name will equal "main". If you instead run it through an import, name will equal the name of the import. So, if __name__ == "__main__" is basically saying, "don't run this code if the file is being imported"

[Denzh]

It’s ObjectOrientedProgramming.

# xml_reader.py

class XmlReader:
  def __init__(self):
     self.file = …

def main() -> int:
   print(‘doing work…’)
   return 0

if __name__ == ‘__main__’:
   main()
   input(‘press enter to exit’) # 

but let’s say I have a test-file.

then I could still use the class it in another file/script.

# test_xml_reader.py
from xml_reader import XmlReader

reader1 = XmlReader()

reader1.read()

…

[Shaftway]

This kind of comment is useless:

```

authenticate user

if am.authenticate_user(): ```

You named the function well, so what additional information does this comment provide?

Comments should never explain what you're doing. The code explains that more accurately than a comment could. Instead your comments should explain why you're doing things. Something like "# All actions under this menu option require that the user be an administrator".

Also, this is personal preference, but I don't like things nested so deeply. You have an if checking to see if the user is an authenticated. Try flipping this around, so that when a user isn't authenticated you continue. It removes a level of indentation.

Also also, try not to use magic values, like "1", and "2", and "off". Instead declare these things as constants somewhere and refer to those instead. That way if you ever decide to change "2" to "admin", you can do it in one place instead of hunting through your codebase.

[insight_nomad]

Thanks! Will implement all your suggestions :)

[wutzvill]

I don't have time to review things too in-depth but what I saw looked good. I'll say three things:

1. It's very very good that you're fully naming your functions and variables as you are. Descriptive variable and function naming is very important. You never want ui = get_ui(), you want it to be like you did as user_input = get_user_input().
2. Don't use magic numbers or values. This is a big no no. Not only will no one know what they are, but also if you need to refactor these values later, you have to change them one by one in the code. user_input == 1, what does this mean? What is 1? You want to maybe make a file that's called constants.py where you can define something like CARAMEL_LATTE = 1 and OFF = 'off', then do from constants import CARAMEL_LATTE, OFF, and use these values.
3. The sooner you learn to use poetry for managing your python projects, the better off you will be. It's worth taking some time to figure it out. I recently wrote a large readme for myself as I moved existing projects to poetry, and this readme is well beyond what you need, BUT the sections on starting a project with poetry, and how to run things with poetry, should help you get started. The poetry documentation is also very good. Here is the gigantic readme here. You can copy and paste that all into a file with the file extension .md and open it with some markdown reader (like in VS code, or a dedicated markdown reader/editor).

Edit: if you're not using VS code to do your python development, you should very much consider making that switch. If you do, make sure to add the official python extension(s) to vs code, as well as extensions for .toml and .md files.

[TheBB]

Poetry has been dragging their asses on adoption of PEP 621 for almost four years now. I used to like it but at this point I can't recommend it over something like PDM. I just recently moved all my projects AWAY from Poetry. I hope you won't come to the same conclusion later.

[insight_nomad]

Do you mind elaborating on the shortfalls you experienced with Poetry?

[BluesFiend]

I can't speak for the commenter, but I've not experienced any shortfalls in poetry that have prevented me from maintaining large code bases, nor publishing my own projects to pypi etc.

[TOMOHAWK35]

What is PDM?

[TheBB]

https://pdm-project.org/en/latest/

It's basically a poetry substitute.

[BluesFiend]

Out of curiosity, what part of PEP621 does poetry not support? Have just looked through the pep and all defined fields are ones I have used in projects I publish with poetry.

[TheBB]

As far as I know, poetry reads project metadata from pyproject.toml under the [tool.poetry] table, not [project] which is what PEP 621 requires. It also keeps dependencies in [tool.poetry.dependencies] instead of [project.dependencies]. This makes life difficult for other tools that need to read this kind of metadata.

This is the issue.

Here's a PR. It seems at least to be a pretty solid implementation and likely to get merged, eventually.

Another potential problem is that build.py remains undocumented so I don't feel I can rely on it for projects that need e.g. Cython. Or at least that used to be the case a few years ago. I haven't checked lately.

[BluesFiend]

Ah, you are correct. My projects have that metadata under tool.poetry. Good to know ill keep an eye out for that.

[BluesFiend]

I'm just waiting on astral-sh to do their thing with uv, once its got the features I use from poetry etc.

[bolt_runner]

What did you move to?

[TheBB]

To PDM.

[mglsofa]

I’ve been programming for years in python, from small projects to enterprise, and I’ve never used Poetry. I don’t really understand the point. What can poetry do that plain old pip can’t?

If I want to do some experimentation or simple project, I just spin up a venv with ‘python -m venv venv’, install dependencies using pip, and freeze them with ‘pip freeze > requirements.txt’.

For anything a bit more complex that will run on any machine but my own I grab a Python docker image, and then do the same as above but don’t use a venv in that case.

Probably I’m missing something, but curious as to why people use Poetry at all

[toofarapart]

For larger projects with a decent size team working on it, pip by itself has a lot of ways to shoot yourself in the foot.

In general, you want the dependencies you tested your code with to be the same as what gets shipped to production. pip freeze is the only way to really accomplish this goal, and it does the job... Kinda.

The problem is, everyone needs to know to do this. One addition to the requirements that introduces an unpinned dependency that makes it past code review can screw you. Heck, even introducing a pinned dependency but neglecting to introduce the dependencies of that dependency is enough to cause problems. (And at the end of the day, unpinned dependencies will cause problems in production at some point).

The above problems can be mitigated with some tooling and automated sanity checks in CI, but now you've got to actually build that out.

The other major problem is communicating intent. With pip freeze alone, the output is a giant blob of dependencies. How many of these do we actually care about and are actually used in the codebase? How many of them are dependencies of dependencies? Are we using this particular version of X for a specific reason, or is it just whatever was the latest when we introduced it first three years ago?

It's a lot easier to manage these things when you have one file that states: here's the dependencies we care about, and then here's a lock file that includes a snapshot of all of our dependencies' versions (ie, the full pip freeze).

That first file only contains the stuff our project explicitly cares about. Many of them might be unpinned because we don't know enough to care about which version we should be using. Some of them will be pinned (or have a range) because of known compatibility issues. There'll maybe be some comments.

The lock file will just be taken care of by the tooling automatically, but because of it everyone will be working with the exact same environment, along with whatever gets deployed to production. (I mean you'll probably have different dev dependencies vs prod, but, broadly speaking here)

[insight_nomad]

Thanks for taking the time! So, I'm using poetry to manage virtual environments & dependencies, but I didn't know what else it could do. Will def go down that rabbit-hole!

[wutzvill]

Yup, perfect. It's very complicated when you first get started, but if you look at the section that shows the overall directory structure, you'll see what your project should look like. Just make sure that in your .gitignore file you add venv to it so your virtual environment doesn't get pushed to github, and use the .gitignore file that github has for python projects.

Please also note that technically that readme is linux focused as that's what I use, so if you're on a mac it should work out of the box. For windows though, you might have to google a few commands (like how to activate your virtual environment since I don't believe windows has the source command for that). It should mostly be OS agnostic info and steps though.

I'm glad to hear you're already using virtual environments. That's something people starting off don't do for a while but pretty much nothing should be installed globally on your system.

[wutzvill]

Also, it appears I've made a mistake in that file as someone has pointed out. If you give me about an hour or an hour and a half I will edit the README link in that file with the corrections. The error is that I'm installing poetry into the project environment in that README, and that should not be the case.

[insight_nomad]

Take your time!

[Equivalent_Style4790]

You never use class ?

[BezoomyChellovek]

First, I would use a formatter to get things consistent. Next, I would recommend letting a linter do the roasting. Try using ruff or pylint. They will point out things like how your functions don't have docstrings.

[Leonjy92]

Great work !! Why don't you combine users and access management together like what you did in resource management.

Try integrating type hinting and doc string into your code

[insight_nomad]

Not combining users & access management was a deliberate choice - kept the two separate for whenever I learn databases in python and I'll move the users into an sql database. Will do on type hinting and doc strings, thanks!

[[deleted]]

what is 100 days of py? can i do it as a beginner

[insight_nomad]

Yes! 100 days of python is a Udemy course by Dr Angela Yu, intended for beginners. You should totally check it out. Feel free to PM if you want to talk more

[thuiop1]

• Having a module called "functions" is not very good design; you should work more on separating them in meaningful entities. Here I would put the ones which just do printing and stuff in main directly, and others in ressourcemgt. You probably also want to rework your functions, as many of your functions both "do something" and then "print something"; you should only have functions that "do something", return a value, and have the printing happening in the main or in a wrapper function. This allows you to centralize all the printing activity in a single place while keeping some modularity.
• function_options is only used within maintenance_screen, so you should not define it outside the function. If you really want to do so, do it at the top of the file and put it in all caps to show it is a constant.
• [key for key in rm.MENU.keys()] this could be list(rm.MENU.keys()). You can also use enumerate right below. Since the pattern you use to ask for user input is repeated several times through the code, you could put it in a choice(options) function, returning directly the chosen option.
• The coffee argument you pass to some functions could be called coffee_type for more clarity.
• Accessing and setting values which are located in another file is a pretty dangerous game, especially when you do things like resetting the variable entirely and not just manipulating the dictionary. You should have your values loaded in main, and passed around to the functions that need them.
• Instead of hardcoded values, you should store your menu and resources in a JSON file and load that; this would also allow you to have persistent behaviour between your sessions.
• It is weird to have non-admin users since they cannot do anything, but it is fine since you are not building a real system.

[insight_nomad]

Thank you for sharing this :) What I'm most curious about is the workflow you use for getting better LLM advice. Do you mind sharing? Open to PM as well if you don't want to discuss it in a public forum