Feature/submission #1
Conversation
There was a problem hiding this comment.
Thanks for this. Most of my comments are just for my understanding, or about stylistic choices.
Good, readable code. I could clearly understand what each module/function is doing, and I hvae a high-level of confidence that this will work, without having to install and start it.
| * Module dependencies. | ||
| */ | ||
|
|
||
| var app = require('../app'); |
There was a problem hiding this comment.
The use of var in this file makes me think it is copy/pasted boilerplate 😉. I have no problem with that, and appreciate that this is supposed to be a quick. But just for info: I would usually be asking for this to be changed, before reviewing the rest of the PR.
There was a problem hiding this comment.
That said: since this is intended to be a bin file: you could have a good reason to use var?
| const saveData = require('../middleware/saveData.js') | ||
|
|
||
| // Route to add a new city | ||
| router.post('/cities', validateCityId, loadData, async (req, res, next) => { |
There was a problem hiding this comment.
I really like the way that you've composed middleware in your routes — really good separation of concerns, and makes it very readable.
| return response.data; | ||
| } catch (error) { | ||
| console.error('Error fetching weather data from API:', error); | ||
| throw new Error('Failed to fetch weather data'); |
There was a problem hiding this comment.
Good. Let middleware handle this.
| req.data.cities[cityId] = cityWeatherData; | ||
| next(); | ||
| } catch (error) { | ||
| res.status(500).send(error.message); |
There was a problem hiding this comment.
I'd like to understand why errors are being handled in individual routes, rather than letting middleware catch them. From what I recall, Express allows you to call next(error) and it will take care of any issues with asynchronsity?
| // Utility function to read data from file | ||
| function readDataFromFile() { | ||
| try { | ||
| const data = fs.readFileSync(datastore, 'utf8'); |
There was a problem hiding this comment.
I might be able to learn something here: are there advantages to using the blocking versions of readFileSync and writeFileSync over the standard async readFile and writeFlie?
| const data = fs.readFileSync(datastore, 'utf8'); | ||
| return JSON.parse(data); | ||
| } catch (error) { | ||
| console.error('Error reading data from file:', error); |
There was a problem hiding this comment.
I'm not saying this is wrong, but interested to know reasons for letting this function fail silently, rather than just letting it throw?
Final submission for the Medical Tracker test.
All endpoints have been implemented and a test created for each.
Increased test coverage would be something to add in future given more time but all routes have coverage.