Fixed swmm_setNodeInflow #45
Conversation
| } | ||
| } | ||
|
|
||
| return(inflow_setExtInflow(j, param, type, tseries, basePat, |
There was a problem hiding this comment.
It would be more clear if the call to inflow_validate() happens here as opposed to nested within inflow_setExtInflow().
Also seems to be a little confusion about separation of concerns - parse, validate, and set.
There was a problem hiding this comment.
The swmm_setNodeInflow API function needs to be validated too. So the data is prepared by either the parser or the API function.. Then validated. .then pushed to the data model.
The validation step assigns the units conversion factor too (cf). Why is this important the way it is now? Future expansion. The swmm_setNodeInflow function can be expanded later to allow mass/concentration inflows.
Honestly, that's about the only thing the validation features does
| type = FLOW_INFLOW; | ||
| cf = 1.0/UCF(FLOW); | ||
| } | ||
|
|
There was a problem hiding this comment.
Not clear why this was moved to inflow_validate(). This is an assignment based on a value parsed from the input file.
There was a problem hiding this comment.
See my answer below
The validation step assigns the units conversion factor too (cf). Why is this important the way it is now? Future expansion. The swmm_setNodeInflow function can be expanded later to allow mass/concentration inflows.
| { | ||
| *cf /= LperFT3; | ||
| } | ||
| } |
There was a problem hiding this comment.
There isn't a lot of what I would classify as validation going on in the original function to begin with. So I guess part of the separation depends on how you want to present the API to the developer.
There was a problem hiding this comment.
I agree that it's not a huge amount of validation. I still think separating is sensible.
| inflow->basePat = basePat; | ||
| inflow->extIfaceInflow = extIfaceInflow; | ||
| } | ||
| return(errcode); |
There was a problem hiding this comment.
This is a good separation of the set concern from the other functionality.
|
@stoiver, do you have any issues/concerns with the updated function? |
Upstream dev
This addresses #40
Also a few other enhancements (Next time these will be separate branches/PRs - Sorry In advance).
chararray.I broke apart the function that originally parsed lines of the input file into three pieces: