james-d12 / whitehatbootcamp Goto Github PK

Really comprehensive solution James, well done.

• I see that airports-config.yaml is read in, given we have the @Swagger annotations on the methods you can remove the paths property from your yaml file. Check your docs still generate ok
• I would return a 201 Created for a POST (create of a resource)
• Really good to see validation of input, can you extend this to whitelist against a regex & check length of input?

Just thinking more about server-side validation. Can you look to define a json schema with validation rules using https://ajv.js.org/guide/getting-started.html. JSON schema is a standard way to represent the structure of a JSON object in a language neutral way.

There is more info about this library here:
https://www.linkedin.com/pulse/server-side-validation-using-standard-json-schema-node-oyedayo

As ever, beautifully laid out code, well done. Only thing I would add is error handling with try/catch as per https://javascript.info/async-await#error-handling

Great job James, well done. Comments & questions below:

Accessibility

• clear font
• + button on Home Page does not have aria-label to explain what it is doing. Also should be nearer the start of the page to save screen reader having to read the whole page before finding it

General

• use of HTTPS & certificates, nice!
• good structure of code into separate directories
• great to see use of fetch() API & understanding of async code
• great to see evidence of Cypress system tests
• I notice you are using PUT for edit, I like it, very RESTful. However, as you are using PUT you could remove the /edit from the url as PUT implies an update. /edit only needed if your APIs just use GET and POST
• need to include client side validation for form data (e.g. check valid url)
• need to include server side validation

Questions:
How are you using cookies/sessions?

James this is absolutely excellent work! Also been nosing through your other repos, didn't realise you had such great experience with C, C++ and C#, would love to chat more about that another time.

A few comments below:

• love the neat directory structure with clear separation of data, classes and tests
• excellent attention to detail eg. use of const, well laid out code, good naming of tests
• diagram for Airport, excellent
• Airport class, I am wondering whether in Java/C++ line 14 would cause a problem - at this stage the object isn't fully constructed as the constructor has not completed. Javascript likely to be much less fussy but if you get time can you check how C++ would handle this? (Or we can setup a quick call and I can try in Java).
• Airport class line 31, JSDoc param should be a string
• Airport class - ensure you add class JSDoc (describing what the class does) and also text describing each method e.g. https://github.com/WhiteHatLearningProducts/swe-solutions/blob/main/bootcamp/wk1/day4/Airport.js
• Plane.test.js - what is this expected? No comparison?
• Crew.test.js - I would separate our tests for failure & tests for success in their own tests

Questions (please reply on Slack)

1. How much coverage on tests do you have? Please upload a screen shot (sorry haven't got time to clone & run myself) unless you can suggest an easy way where I can just click on a link in your repo to the index.html? I wonder if you reference it as a link in the README.md markup if that would work? If this works, could you try same for JSDoc? I know this means we'd have to change gitignore to include out/ and coverage/ dirs but worth an experiment!

what you did well:

• UML diagram excellent, really accurate
• excellent to see use of Sequelize
• good use of response codes especially 409!
• great to see use of Supertest
• great to see server-side validation

areas for review:

• naming of REST resources should be plural e.g /users/${email}/accounts (give me all of the accounts belonging to this user)
• quite a lot of layers of indirection for the server, I would rename Server/Server to server/src and move API.js into this folder
• I'd also rename API.js to something else as I assumed routes were in here - hard to find your routes, do you need to name them Controllers? Client folder has a better directory structure.

James, as ever, this code is absolutely superb. Specific things I love:

• use of Utility class to generate an id
• really well thought out design eg. only ElectricScooter has a chargeLevel
• JSDoc complete
• ScooterHire, thought/effort put into logic
• async functions to read scooter info - wow! Will schedule time for you to talk me through this.

A few comments:

• line 46 of ElectricScooter - does this work? Super normally only used in constructor
• Customer - no need for line 26

Huge well done.