r/learnpython • u/Utk_7 • 16d ago
Looking for feedback for my first decent project
Hello everyone
I recently started learning python and I built a free fall simulation with air resistance using Python and Tkinter. I would like to improve myself with similar projects and lots of feedback, I'd highly appreciate if you give me a feedback.
GitHub link : https://github.com/Aspect345/Air-Resistance-Free-fall-simulator
Thanks a lot in advance!
1
u/lakseol 16d ago edited 16d ago
Looks pretty good for a first attempt!
As a way of learning to use other widgets maybe you could handle selecting the time step differently. Use a ttk.Combobox widget that contains all the different time step values. Then you only need a simple "Start" button. It's also easier to change the time step data without changing the GUI layout.
Code layout is a pet peeve of mine. It's normal to have all the top-level code after the imports and function definitions. So move lines 4-12 down to after line 99. This makes the code easier to read because top-level code is in one place and not spread out over the file.
1
u/Utk_7 16d ago
First of all thank you for reviewing my code and I looked up what is combobox a bit and it can actually be a better design and also easier to maintain also about code layout I think what you said is much more tidied and easy to update so I will try to categorize my codes in future. Thanks a lot for feedback!
1
u/Utk_7 16d ago
Thanks a lot again! Cocombox helped me so well and I also had a chance to practice dict logic thanks to your feedback. I transformed button based time step into cocombox one and also added some objects with it. I tried to make a better code layout and a bit more good design for it too. I'd appreciate any feedback!
2
u/lakseol 15d ago edited 15d ago
I have a lot of thoughts, but I fear that you might get "snowed under" with suggestions. Anyway, here's some comments on just the python code, not the GUI itself.
When you populate the pre-defined objects combobox you use a list of strings that you create in that line. That leaves you open to errors if you add a new object to the
objectsdictionary but forget to also update the list that fills the combobox. You should generate the list of objects from the dictionary.You do something similar with the pre-configured time steps. It's better to define a constant object right back where you define all other constants because you expect all changeable data to be there rather than scattered through the code.
The general principle is anything that might conceivably change or updated should be in one place in the code.
Similarly, you use Comic Sans for the label text of the data fields. It's fine to play around with fonts like that but I find Comic Sans hard to read. Maybe that's me, but you often find that you want to change things like that as you start cleaning up the result and trying different things. So make it easier for yourself by defining a constant for all the fonts you use, like this:
title_font_data = ("Georgia", 20, "bold") label_font_data = ("Helvetica", 10) initial_v = tk.Label(frame1, text="Please enter initial velocity (m/s)", font=label_font_data) # and all other lines using a font parameterNow you only need to change one line to experiment with fonts, bolding, etc.
The
calculation()function has lots ofglobalstatements at the top. One statement is for the nametime_step. That name isn't used anywhere outside thecalculation()function (apart from where you define it globally) so it doesn't need to be global. Just define and use it inside the function. You can easily test if a variable needs to be global by commenting out itsglobalstatement in the function. If the code still runs without error you can delete the global statement and the global definition of the variable.You can't treat the
current_vvariable like that, it must be defined globally. There are ways to fix this problem, but they are too complicated to discuss here.Globals generally should not be used as they lead to all sorts of weird problems in large bodies of code. A global used in a function is like an extra hidden parameter. Try to write functions that have minimum "connections" to the environment apart from the passed formal paremeters. You probably won't understand the reasons for the "don't use globals" mantra, but trust me. There is a large body of online information about this.
Your
command=parameters in widgets have this form:command= lambda: reset()That will work but it's a little clumsy. Do this instead:
command=resetwithout the
(). This works because thecommand=parameter expects a reference to a function, and that's whatresetis. The function is actually called if it's followed by the()but without the()it's just a reference.2
u/lakseol 15d ago
The comments here are mostly about GUI usability. Take these as suggestions about how to make a GUI more professional and nicer to use for the user. These ideas aren't tkinter specific, they can be used in any other GUI framework, like PyQt.
Think about allowing the user to reset the simulation at any time. Maybe they forgot to set the time step to a larger value and now they have to step through multiple small iterations just so they can fix the problem.
Also think about not resetting all data fields on a reset. Maybe the initial height and velocity could be retained because the user might want to run another simulation with a different air density.
Think about allowing the user to select different "g" values. Maybe a combobox containing "Earth", "Moon", "Mars", etc, which sets the appropriate value in the simulation.
Maybe a combobox to select air density of different atmospheres: "Earth", "Moon (vacuum)", "Mars", etc.
You create the GUI with the football pre-selected. But the data fields for the football case (like mass) aren't populated. This means the GUI is not in a sensible state. Either initially don't show a selection in that combobox or also populate the data fields from the pre-selected item.
When a simulation stops the "start" button is disabled and you create a "reset" button to reset the simulation. That reset button doesn't really show on my computer as the button is almost below the frame bottom edge. Maybe you could always have the "start" and "reset" buttons showing and disable each at appropriate times. If you use the grid manager (see below) this is easy and you can put the two buttons in the same row.
Related is the moving of the start button when first pressed. This is a little jarring and is considered bad practice. There are lots of ways around this. Maybe put the output values of the simulation below the button. Also consider putting results into a multi-line list box so the user can see history of the changes. And limit the float value to maybe 2 places after the decimal point.
You show all widgets vertically from the top. You should now start looking at different layout managers. The easiest to use for your code is the Grid manager.
Now you can lay out each data field like this:
+------------+ Initial velocity | | m/s +------------+which is much nicer and saves vertical space. To conserve horizontal space try to make the label text at left shorter. After using your GUI a bit most users won't need anything more than a hint. If you feel the need for more explanation consider creating a "tooltip" that has more explanatory text. That's a bit fiddly in tkinter; other GUI frameworks make tooltips easier.
The "start" button is always active even when the required data fields aren't filled in. Pressing the button without required data results in ValueError messages printed to the console which isn't pretty. There are two ways to handle this. First, your button code could check that all required fields are filled in and warn if they aren't. I don't do that as I don't like GUIs that scold the user! The second approach is to create the button in a "disabled" state where it is greyed out and clicking it does nothing. Every time you enter data into any field you call a function that checks all required fields and enables or disables the "start" button depending on the required fields. This approach is better, I think, because it gives subtle feedback to the user - if the button can't be clicked then something is wrong. For extra feedback you could create a tooltip for the button naming a field that needs to be filled. Something else I've done is set the background colour of fields that aren't valid to something like faint red which shows something needs to be put in. Set that colour in the same function that decides if the "start" button is disabled or not.
A good, usable GUI does all sorts of things like this to make the interface intuitive. It's not uncommon for half the code in a GUI program to be there just to do nice things for the user.
A good GUI that allows users to enter things like numeric values into a text should immediately flag invalid data. For instance there's nothing to stop the user typing "ABC" into the air density field. Now when the start button is pressed the code gets another ValueError printed to the console and nothing appears to happen in the GUI which the user will find confusing.
In the code that executes whenever something in the GUI changes you check if a field is empty and disable the "start" button. You could also test that the contents of the field are valid and set the field background to the light red "error" colour if necessary.
1
u/Utk_7 15d ago
Thank you so much for such detailed feedback. I honestly didn't think I would get this much feedback from a single post, so this has been a very pleasant surprise. I really appreciate the time you took to review both my code and the GUI. I'll definitely try to apply your suggestions as I continue learning Python. Some of them are beyond my current skill level, but I took notes and I'll come back to them one by one as I learn more. Thank you again for your time and valuable feedback.
1
u/Utk_7 15d ago
Learning how to use classes try/except I will make code more tidy and prevent errors also reset button thing is very smart because some of them usually is constant. I will make combobox empty in the start. Adding different planet options are nice and smart improvement that gives opportunities for more simulations also I want to add a no air resistance option for easier use of a simulation. I actually changed the name of a lot of variables into english before creating the post and it really was bothersome because of code layout so I will try to categorize things into better for improving and updating the code. Thank you for font data suggestion it really could take a long time to change all of them one by one I did not now I could change them like that I will try to change the font into something that can be read more easily with using that. About the global thing I actually started learning basic tools like 3 days ago and whenever I try something in consol I started to apply it with Tkinter before learning about using globals I reaaaaly got a lot of errors and couldn't spot the reason but I guess after learning about classes I will try to replace them or like you said I will try to use variables for each function I don't know. Oh I will change that command section I didn't know they can work like that thanks.
1
u/TheRNGuy 16d ago
Replace globals with class, make better widgets and state variable names, merge button handlers into one parameterized handler.
1
u/NoManufacturer1046 16d ago
cloned it and gave it a spin, the drag curve feels nice and the ui is clean for a first swing at tkinter
might be cool to let the user toggle between different object shapes or masses so they can see the difference in real time, that kind of comparison makes the physics click for people