Skip to content

Solution for uncreative parents#17

Open
ctorrington wants to merge 6 commits intoalexnaylor99:mainfrom
ctorrington:main
Open

Solution for uncreative parents#17
ctorrington wants to merge 6 commits intoalexnaylor99:mainfrom
ctorrington:main

Conversation

@ctorrington
Copy link

@ctorrington ctorrington commented Apr 9, 2023

Readme fixes + code solution :)
for @alexnaylor99 & everyone to critique.

@ctorrington ctorrington marked this pull request as draft April 9, 2023 18:50
@ctorrington ctorrington marked this pull request as ready for review April 9, 2023 18:51
Copy link

@HenryXanderTalent HenryXanderTalent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positive:

  • Good use of the class feature.

  • Good use of regular expressions.

  • Good use of comments and doc strings, all methods are easy to distinguish.

  • Good consistency with naming for methods and variables.

  • Good use of lambdas.

  • Well structured with consistent spacing and indentation.

Recommendations:

  • Nice story at the beginning but doesn't explain clearly what the app does or how to use it. I noticed it does instruct the user once started so I guess its not such an issue.

  • No readme.

  • App fails if you enter a non digit and has no error handling. Possibly use exception handling and a system call import to restart the app or add another validation method for this.

Overall:

  • Well formatted with advance python techniques and does the job!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants