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

Coding Comments #100

Open
1 of 13 tasks
sngillis opened this issue May 7, 2024 · 1 comment
Open
1 of 13 tasks

Coding Comments #100

sngillis opened this issue May 7, 2024 · 1 comment
Assignees

Comments

@sngillis
Copy link
Collaborator

sngillis commented May 7, 2024

Mohamed,

First off, GitHub / GitLab has Issues (this is an Issue). It's like JIRA lite (if you are familiar with JIRA). It's a handy way to do basic Feature and Bug tracking. When it's your code being developed alone, using Issues isn't necessary. When collaborating, Issues are a very handy way to keep everyone up to date.

Here are some initial thoughts on primary concerns for Splash:

  • You need more comprehensive commenting.
  • Not just a handful of line comments, but also public documentation (I can show you the basics).
  • Especially if you have any equations or anything that should be clearly cited (like an equation or algorithm from a paper).
  • The Python Sphinx library can take public documentation from the source code and assemble a rather nice web site (e.g. very similar to how JavaDoc puts together a full web site for a Java project). You can use the produced site for both developer documentation, but also for user documentation.
  • We need to talk about structure (not architecture, just some basic structure).
  • Splash.py should not be 2000+ lines long. A great deal of the content of the file should be broken out into other files (ideally by related functions / data).
  • Especially GUI elements. I need to look at how Python does GUI threading.
  • Also, Splash.py needs to be arranged better so a person reading it can easily figure out where the execution starts and goes.
  • Finally, the file 'splash.py' should have a 'class splash' and not a 'class TerminalApp'. I know Python doesn't care like Java or C++, but it is a good idea for a file with a single class to have the file name and class name match. It helps alleviate confusion.
  • You need Error handling. It's almost as much of a pain to put together error handling as it is comprehensive documentation, but you will thank yourself when crap breaks and the error messages you wrote give you clear indications as to what broke,

These are secondary concerns:

  • You can use some String management. Basically, assign Strings to variables at the top of the file or in a separate file, and then use the variables when passing a string to something. It makes it easier to track down Strings for editing, and if you ever decide to enable internationalization, having all the Strings in one place makes it easy to translate to another language.
  • Python has Enumerations, and Enumerations are AWESOME!. I'll need to look over things more closely for where Enums would make sense, but there is always somewhere that can use Enums.
  • Why does Splash have a terminal window again?

That's it for now. If you want to chat, let me know. Or we can just communicate through the issues.

@mohamedalysayed
Copy link
Owner

Fantastic feedback! thanks a lot Shane. We've been working on a brand new release which will have a much better architecture ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants