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 Comments Luke Fanguna #9

Open
Luke-Fanguna opened this issue Nov 11, 2023 · 12 comments
Open

Code Review Comments Luke Fanguna #9

Luke-Fanguna opened this issue Nov 11, 2023 · 12 comments

Comments

@Luke-Fanguna
Copy link

For readability, I would add this in the connection.execute() parameter to show where it is being used. If it is a global variable, it would look like it is being used somewhere else.

add_peepcoins_query = text(

@Luke-Fanguna
Copy link
Author

It looks like when the queries need more variables, you change the format of the json to insert strings. For consistency, I would keep the same format. Whether it needs two or ten variables, I'd keep the same format to make it cleaner

@Luke-Fanguna
Copy link
Author

This function is really dense and the formatting is hard to follow. Have consistency in writing queries. I've noticed that other queries are in proper SQL format.

query = text("SELECT valid FROM coupons WHERE id = :coupon_id")

@Luke-Fanguna
Copy link
Author

Not the biggest fan of separating "query" and then redefining it after each query. It would be easier to read if they were put into the text() and reduce the redundancy.

query = text(

@Luke-Fanguna
Copy link
Author

Luke-Fanguna commented Nov 12, 2023

This part seems like it is asking for too much. It takes in the 'Routes' object but only uses 'location'.

def report_route(route_to_report: Routes):

@Luke-Fanguna
Copy link
Author

Could you make the queries more compact in terms of formatting. Very large function just for two queries. Also, I don't see a point to have PEEP_COINS_FROM_POSTING_REVIEW if it will be 5 every call.

def post_add_review(review_to_add: Reviews):

@Luke-Fanguna
Copy link
Author

Now I see the usage of the global variable. I'd say there is no harm is rewriting queries just to make it more readable.

add_peepcoins_query,

@Luke-Fanguna
Copy link
Author

Fix the indenting here to make it seem more uniform. Other queries look good.

SELECT id

@Luke-Fanguna
Copy link
Author

Majority of these queries are majorly indented. Not the biggest fan. I'd say to align it like the other ones.

INSERT INTO route (name, user_id, location,

@Luke-Fanguna
Copy link
Author

I'd say that this query is quite large. You can potentially create different tables for organization.

https://github.com/n-shinde/PeakPeeps/blob/c5e21b2277ebdb75b013e12e53dcb69739189c4a/src/api/routes.py#L39C37-L39C37

@Luke-Fanguna
Copy link
Author

If the admin.py file will not be used, you can just delete it.

@Luke-Fanguna
Copy link
Author

Will this output always be "True" and "Reported"? If so, I don't see the use in having those variables.

status = "Reported"

@Luke-Fanguna
Copy link
Author

Commit messages like 'done maybe???' and 'try test again' is too vague. not enough info from those commit messages.

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