Regarding #17: There are a few noteworthy small changes that might improve overall code quality.
When starting the program for the first time, the data directory would not be created. Instead an error was thrown and one had to manually create it. Now the folder will be created automatically.
In harvestOcInstance.js the initStoredData does not load the data in an asynchronous manner. We could use await Promise.all(...) to change that
There are two cases throughout the codebase in which a new Promise(....) is explicitly created inside an asynch function. In my opinion, since an asynch function will always wrap the result/error in a promise, it would be better to restrain calling of new Promise(....). This leads to better readability, uses the advantage of asynch functions and prevents forgetting to explicitly resolve() or reject() given promise.
I exemplary changed the storeData() function in data-storage.js.
Regarding Error-Handling (#19 ): Besides refining a few error-messages, there currently is a bug that we ignore an thrown error, when trying to create an existing ES-node (#15). I proposed a solution in #20.
UPDATE - Ready for review
Except the known bug from #15, all improvements 1-4 are implemented. In general the program should behave in the same way as before. I added two noteworthy additonal things:
General Error-Handling: I created custom Error classes, that will be thrown in the appropriate cases and that cause fitting Error Messages from the logger. For now i put the js-file in/models/errors.js.
await vs then: Throughout the code a mix of the keywords await and then is used. I believe most of the time, we can simplify the code a lot by only using the await syntax. We should not lose any parallel processes, since in most of these cases we wait for the previous operation to finish anyways. I changed the corresponding code in harvestOcInstance.js. We can probably simplify quite some more code, but I would not prioritize that for now.
--> Code is ready for review and can be merged if approved
ffeyen
commented
3 years ago
See Update
Regarding #17: There are a few noteworthy small changes that might improve overall code quality.
When starting the program for the first time, the
datadirectory would not be created. Instead an error was thrown and one had to manually create it. Now the folder will be created automatically.In
harvestOcInstance.jsthe initStoredData does not load the data in an asynchronous manner. We could useawait Promise.all(...)to change thatThere are two cases throughout the codebase in which a
new Promise(....)is explicitly created inside an asynch function. In my opinion, since an asynch function will always wrap the result/error in a promise, it would be better to restrain calling ofnew Promise(....). This leads to better readability, uses the advantage of asynch functions and prevents forgetting to explicitlyresolve()orreject()given promise. I exemplary changed thestoreData()function in data-storage.js.Regarding Error-Handling (#19 ): Besides refining a few error-messages, there currently is a bug that we ignore an thrown error, when trying to create an existing ES-node (#15). I proposed a solution in #20.
UPDATE - Ready for review
Except the known bug from #15, all improvements 1-4 are implemented. In general the program should behave in the same way as before. I added two noteworthy additonal things:
General Error-Handling: I created custom Error classes, that will be thrown in the appropriate cases and that cause fitting Error Messages from the logger. For now i put the js-file in
/models/errors.js.awaitvsthen: Throughout the code a mix of the keywordsawaitandthenis used. I believe most of the time, we can simplify the code a lot by only using theawaitsyntax. We should not lose any parallel processes, since in most of these cases we wait for the previous operation to finish anyways. I changed the corresponding code inharvestOcInstance.js. We can probably simplify quite some more code, but I would not prioritize that for now.--> Code is ready for review and can be merged if approved