search

fac-17 / NomNom

A web app to find and create weekly meal plans that use common ingredients in order to combat food waste
https://nom-nom-fac.herokuapp.com/
0 stars 3 forks source link

Code Review #94

Open misterrodger opened 4 years ago

misterrodger commented 4 years ago

Greetings, all!

I hope you've had a nice weekend, project is looking good so far, you have put a lot of work into it!

How low can you go?

This is a very cool and useful concept, and the idea itself has the potential to become a very 'meaty' app that would ideally have a few more weeks or months to really build out and test more. So I think for this week the goal is to identify what is truly the minimum MVP, and complete that as soon as possible. Then push any remaining ideas and features (which will be a lot) to later in the week or for a hypothetical 'next sprint.' You have written a lot of good backend and template framework stuff, and built a strong foundation, which is great and will make the project robust. At the moment the user doesn't yet see a lot of that. This is apparently a common situation in development. So for the next few days, perhaps rather than thinking in terms of releasing "one big final project" and expecting (hoping!) it all comes together, I'd try to think in terms of small, iterative, "rapid releases". Get some more 'wins': pull requests you can merge and deploy that the user will see right away, with the foundation you've already built. You've got a DevOps person, so this is traditionally their domain, and anyone else can help them with this. I'd recommend making sure first of all the deployment is working and cool, then try to release something small at minimum 1-2x per day if not more. I think this might help balance the project out in terms of work you've put in vs. the reward, if you are able to simplify what you are doing, and break larger features into smaller chunks you can release sooner. Speaking of deployment, let me know if this presents any challenges. I know sometimes something can 'build' in Heroku and then still fail when you navigate to the page. If you face any problems: - Make sure files are where your frameworks and dependencies are expecting them. Re-read the docs and double-check just to be sure. Most of the time they are looking for an index.js or similar in the root, or they go hunting for it according to a pre-set path. - You might need to set 'engines' to node in the package.json, and you might need a procfile with web: node index.js for Heroku. There are a few other things along those lines you can look up if needed. - Also, the package.json says main is index.js but again this isn't in the root. I think it will find it in the src folder, but worth double-checking all your tech stack (particularly if things are failing), or just move the index to the root. - Also check versions of dependencies: If you're getting failures it's worth checking if anyone recently updated versions, and you can also learn how to 'lock' dependencies. - I see both dotenv and env2 - pick one? - .gitignore the ds_store file? Travis - as I said, try to keep it working (any of the above may help), but I wouldn't let it slow you down too much at this point. ## express app: - You can add middlewares like compression and helmet very easily to help performance and security. You can read their docs, though the default settings are fine for your purposes for now. - A logger is also good to add and might save you getting stuck in places. Morgan is easy to add. ## Architecture All looks good, you did the routes and controllers thing from the Express docs (thank you!) and your framework kind of makes you use the MVC pattern. Angular does much the same, whereas React allows you to be more free with the design pattern, which has pros and cons. A good talking point for this week and for interviews: be sure to read a bit about the MVC pattern and be able to talk about it. ### routes.js This file is pretty Promise-heavy, which is cool and I would continue in this style for this project, but then maybe consier learning and using async/await with a try/catch block for your next project. You need to know callbacks and promises to deal with pre-existing code, but most new components you work on will be written in the new syntax, so try to get some exposure to it and know the pros/cons of each method. The bigger functions uniqueMealPlan and getMealPlans would be good practice to rewrite to async/await, but that's bonus. Maybe something to do over the weekend or next week in your free time, if you're keen. Error-handling in Express looks good, thank you. Maybe give the error pages a little bit of love so the style is the same as the rest of the app? You could copy the banner image from the homepage or something like that. db_query factory looks good. Have you tested all these queries manually to see that they work? First priority is to get a complete route of the app working, but at some point it would be great to have tests for these functions. Having a quick glance over the issues you've recently raised: - If an external API is giving you problems...well, they usually do. It's totally your call, but if it were me, I would cut that part for now and plan to render your own data from your own database in the live app. Another way is to write a short script to fetch the external data and populate your DB one time, and then work with the data from there. If you're already querying from the front end to the DB, that's enough really. The page is dynamic that way, which is all we want. Even if you only have 7 items in the DB, it's still fine for an MVP. - email notification might be better off as a stretch goal unless you have found a quick solution. - Feel free to ignore me on all counts and go for it if you think it is within reach, because you might just pull off something very hard, or you might have to re-adjust, but you will learn either way! You seem very close to rendering the ingredients page (maybe you are doing so already) I'd focus on delivering one of those, which will then be basically a complete 'proof of concept' (MVP). Forms: you have several and they are a bit hanging there without a lot of explanation to the user. Easiest fix is to add good old html 'legend' and/or 'fieldset' (a personal fave). You can also use a graphical solution, or even just a different colour background so the user input area sticks out. In any case I would consider guiding the user to action a bit more. ## Design Overall I think you may have been thinking in terms of "mobile first" for this app, which makes sense in some cases. Maybe for this week maybe consider "what percentage of our users will use this on a desktop vs. laptop" and try to hit that balance in your design as well. Right now looks to be about 75%+ mobile...do you really expect 75% of your users will be mobile, or will it be closer to 50/50? I don't know either, I'm just asking...it's worth thinking about. Either way, a quick fix of max-width in the 'app' part of homepage would be good to hold it together a bit more. Homepage layout is cool. I notice there are fewer pics in the app than Figma for now...is this still in-progress? I liked the pictures, but it's your call. CSS: good use of grid and flexibile layout properties. A lot has been done on the first page, so again try to balance the product across the pages at this point and give some more love to the other pages. I know CSS can be time-consuming. Try to reuse what styling you can and remember the visual impression matters a lot. BEM convention: I see you. Nice. It's worth reading a bit more about SEO with SSR and beefing up your meta tags and such, since that is one of the advantages of SSR. This one is not something the user will see, but again a good talking point. "So, how are you liking using Handlebars/Mustache?" It is really cool that you are have gotten more SSR experience with this project, and great that you are building a full-stack project with database. Be sure at some point (doesn't have to be this week) to read a couple of articles about the differences between Handlebars/Mustache, and maybe Pug and Next.js as well, that's the kind of thing people tend to ask in interviews. "Why did you choose this and not that?" You don't have to know about these technologies in detail yet, but ideally it's good to say something besides "well that's what they told us to use on the course." Just an overview of these things would be great. ## Focus Try to simplify it. My company suggests to: "think about what one urgent problem you are trying to solve, that people would pay money for right now, and focus on solving that."