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

MISC things noticed in code review #25

Open
lluddenusgs opened this issue Nov 20, 2024 · 0 comments
Open

MISC things noticed in code review #25

lluddenusgs opened this issue Nov 20, 2024 · 0 comments
Assignees

Comments

@lluddenusgs
Copy link
Contributor

lluddenusgs commented Nov 20, 2024

@ahaj @barkermi
Hey guys so awesome notebooks and now that it actually works, we can get into the nitty gritty of reviewing stuff.

First, the main problem is of course the setup which, we want to streamline this is being handled in Issue #24. Once I finish this script, I would like to run this with a fresh user to see how it works, and the tradeoffs of it. I would also like to hop in a call and discuss the end version of the script.

Before that there are 2 things that are relatively quick that can be solved.

  1. There are times where I had to rename cells to make cells work, assuming these are resolved in the working version should be a simple push. If not I can help push a version of mine once all is renamed if that's needed. As mentioned if we can have this be userinput this could be ideal.

  2. Code syntax: you guys did a really solid job of implementing standardized comment and code structure throughout.
    Some comments need to better communicate the purpose of the code.
    Bad Comment example
    //Create a new object of the MyClass class MyClass obj = new MyClass();
    Good Comment example
    // Instantiate a MyClass object to handle user data processing MyClass obj = new MyClass();
    Explains the thing you are instantiating and the use for it.

A common comment I see is describing the function when the function name does this.
readfile(example.txt) #reads example.txt

this can be done without, cause ideally this is where our naming conventions make this easy to understand without the comments. We can make these changes overtime.

In terms of coding optimization those go into more detail so I'll make separate issues for those and will be much more guided. :)

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

3 participants