drew3110
commented
3 years ago Hi guys, I went through the chaincode and took some notes.
Important high level point: the data model is built so that you have 1 shipment which contains exactly 1 measurement.
Every time there's a new measurement you create a whole new shipment. I think it makes more sense to have one shipment which contains multiple measurements, without creating a new shipment object every time. It can be some kind of list, like an array. This would also make it easier to retrieve history.
I think the current implementation might also create a problem in hardware requirement: if you create a new object with every transaction you're basically demanding the system to keep track in the state of all those objects: this would make RAM consumption to increase indefinitely.
It might not be the case if HLF implements some sort of management to avoid this situation, but I don't know anyone.
I think it's valuable to test this, consider to write a script to automatically submit hundreds/thousands of txs and register RAM consumption to see if it decreases / gets steady at some point or not.
More specific comments:
- There's already a check for "no shipment" in getShipment
- You have a function to check existence but you actually never use it
- You check this differently in different functions, sometimes only
if (!shipment), sometimesif (!shipment || shipment.length === 0)and nowshipment && shipment.length > 0. I find the last one the most elegant, though a bit less readable. Maybe check if there's a convention (and take advantage of this function you wrote instead of manually checking every time).
- RegisterSensor should probably rather return the sensor ID or error, instead of true/false like if it verified if a sensor is registered.
- I suggest to rename it to
sensorIsRegisteredto avoid confusion
- This isn't filtering anything, afaiu
- Mapping should include the times of the measurements too
- With an eye to the future, timestamp should probably be passed as explicit parameter so that it's set by the IoT device. This way the IoT device can buffer some measurements when there's no internet connection and send them later registering the right time.
- Should the function return only temperature or the whole measurement object?
- I don't understand the logic of this section. If you want to check that the temperature is in range, why don't you just do
return isInRange(newVal, minVal, maxVal)? - That being said, why do we even want to have an ad-hoc function to check if a value is in range?
- And why do we want to treat temperature as a String instead of a float (or just an integer)?
Other smaller comments:
- You're throwing generic Error a lot. I don't know enough TS to know if this is normal or if there should be a more specific one.
- The naming convention for functions is
likeThisName(), notLikeThisName()both in JS and (probably) in TS. It'd be nice some small refactoring here and there :)
Feel free to comment further if you need more and/or have questions 😊
gwesseling
Hi Andrea,
Can you review the written TypeScript code for the chaincode (located on the chaincode branch)? Let us know if you find something that we can improve.
Thanks!