updated node stats and system stats functions #36
Conversation
src/massbal.c
Outdated
| case 9: value = massbal_getFlowError(); break; | ||
| case 9: *value = massbal_getFlowError(); break; | ||
| // default | ||
| default: errorcode = ERR_API_OUTBOUNDS; break; |
There was a problem hiding this comment.
I would prefer a more descriptive error code. Something "Invalid parameter code" The more descriptive the error message the easier it is for the user receiving it to diagnose and solve the problem on their own. That's why it's important.
src/massbal.c
Outdated
| // Cumulative Dry Weather Inflow Volume | ||
| case 0: value = FlowTotals.dwInflow * UCF(VOLUME); break; |
There was a problem hiding this comment.
Would prefer enum variable used for case switch.
src/stats.c
Outdated
| // Check if object index is within bounds | ||
| if (index < 0 || index >= Nobjects[NODE]) | ||
| { | ||
| errorcode = ERR_API_OUTBOUNDS; |
There was a problem hiding this comment.
More descriptive error code. Consider "node index out of bounds".
src/stats.c
Outdated
|
|
||
| switch (element) | ||
| // Check if Open | ||
| if (swmm_IsOpenFlag() == FALSE) |
There was a problem hiding this comment.
The flow of control here is not what it should be. As soon as an error condition is encountered you want execution to stop and have the error returned. But this logic continues to check for errors. What it multiple error conditions are present? You are handing back information on the last error condition encountered not the first encountered.
Handle the most severe error condition first with an if statement, then the next most severe with an else if, and so on until all error conditions have been checked, then use an else to enclose the block of main functionality.
There was a problem hiding this comment.
Ok! That's a better flow
src/stats.c
Outdated
| case 16: value = StorageStats[subindex].exfilLosses * UCF(VOLUME); break; | ||
| case 16: *value = StorageStats[k].exfilLosses * UCF(VOLUME); break; | ||
| // Default | ||
| default: errorcode = ERR_API_OUTBOUNDS; break; |
src/toolkitAPI.c
Outdated
| // Check if in bounds | ||
| if (type >= 0 && type < 10) | ||
| if (paramtype >= 0 && paramtype < 10) |
src/toolkitAPI.c
Outdated
| return(0); | ||
| } | ||
| else return(ERR_API_OUTBOUNDS); | ||
| int errorcode = massbal_getRunoffTotal(paramtype, value); |
There was a problem hiding this comment.
Consider changing to simply return massbal_getRunoffTotal(); This is better because we get the same functionality without the definition of a local variable.
src/toolkitAPI.c
Outdated
| return(0); | ||
| } | ||
| else return(ERR_API_OUTBOUNDS); | ||
| int errorcode = massbal_getRoutingFlowTotal(paramtype, value); |
There was a problem hiding this comment.
Consider changing to simply return massbal_getRoutingFlowTotal(); This is better because we get the same functionality without the definition of a local variable.
src/toolkitAPI.c
Outdated
| return(0); | ||
| } | ||
| else return(ERR_API_OUTBOUNDS); | ||
| int errorcode = stats_getSubcatchStat(index, paramtype, value); |
There was a problem hiding this comment.
Consider changing to simply return stats_getSubcatchStat(); This is better because we get the same functionality without the definition of a local variable.
|
@michaeltryby, these are good suggestions. It's a good idea for me to add the enums for these. I'll post changes soon |
|
@michaeltryby, i addressed the bulk of your suggestions here. Let me know what you think about my enums.... I suspect there might be some name changes there. |
|
@michaeltryby, Please review the updates |
|
@michaeltryby Thanks! merging it now. :-) |
Addresses #35 #31