search

n-shinde / PeakPeeps

Group Project for CSC 365
0 stars 0 forks source link

Code Review Comments Luke Fanguna #9

Open Luke-Fanguna opened 8 months ago

Luke-Fanguna commented 8 months ago

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.

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

Luke-Fanguna commented 8 months ago

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

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

Luke-Fanguna commented 8 months ago

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.

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

Luke-Fanguna commented 8 months ago

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.

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

Luke-Fanguna commented 8 months ago

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

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

Luke-Fanguna commented 8 months ago

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.

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

Luke-Fanguna commented 8 months ago

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.

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

Luke-Fanguna commented 8 months ago

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

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

Luke-Fanguna commented 8 months ago

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

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

Luke-Fanguna commented 8 months ago

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 commented 8 months ago

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

Luke-Fanguna commented 8 months ago

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

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

Luke-Fanguna commented 7 months ago

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