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

Code review #30

Open
bobbysebolao opened this issue Oct 2, 2020 · 0 comments
Open

Code review #30

bobbysebolao opened this issue Oct 2, 2020 · 0 comments

Comments

@bobbysebolao
Copy link

Your project is looking soup-er 🍜 compliment sandwich incoming below... 🥪

1. Great job structuring your code

You've clearly thought about how to write reusable code (i.e. by exporting the calculate function from your global /utils directory, and making that InputBox component) that is also easy to read. Mission accomplished methinks!

2. You can make your code more readable by using const instead of let.

Declare constant variables with const to indicate to someone reading your code for the first time that the variable's assigned value doesn't change. This can make your code even more readable and is a good habit to get into.

An example of where you might want to change a let to a const is on line 14 of src/utils/calculate.js

3. Case sensitive endpoints are confusing

Is it intentional that /myProfile takes you to the profile page, but /myprofile doesn’t? I would suggest making your app’s endpoints case insensitive because domain names are treated as case insensitive (WWW.GOOGLE.COM === www.google.com), and it would be strange if only the slug (the portion after the ‘/‘) was case sensitive.

4. Your routing logic is 👌🏼👌🏼👌🏼

I know you talked about using react-router previously, and if this is something that you would still like to learn then by all means try it out next week! But personally, I think the custom routing logic you’ve written for navigating between pages in App.js is nice and readable as it is. react-router just abstracts detail, but is functionally similar to what you’ve already got working.

So on the one hand, don't be afraid to tackle react-router next week if that appeals, but on the other hand, if it ain't broke, don't fix it! 😎

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

No branches or pull requests

1 participant