Codecov Report

Merging #252 into develop will increase coverage by 0.46%.
The diff coverage is 81.66%.

@@             Coverage Diff             @@
##           develop     #252      +/-   ##
===========================================
+ Coverage    62.55%   63.02%   +0.46%     
===========================================
  Files           53       53              
  Lines        16672    16694      +22     
===========================================
+ Hits         10430    10521      +91     
+ Misses        6242     6173      -69

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a838227...18fe690. Read the comment docs.

@bemcdonnell , thanks for pushing this forward.

@LRossman, I did just a tiny bit of refactoring for saving out to an HSF. Mainly passing the file structure pointer as an argument. I can't imagine this will be a deal breaker - but it seems to make a little more sense this way considering we are opening up this as a new API feature.

@LRossman, I'm also leaning toward changing the function names a little too. hotstart_open has more of hotstart_set_initial_state behavior. And hotstart_close has more of hotstart_save_state. The proposed function names could be simpler, yes, but the idea is that they are a little more descriptive/less ambiguous. Thoughts?

@bemcdonnell as I'm reviewing this PR I noticed that the hotstart.c file being modified seems to come from v.5.1.012 (or maybe 5.1.011) and not 5.1.013 since the latter has a code section starting at line 77 removed whereas your file just has the section commented out. Now this may be moot, since there are no comments in the file marking any changes for v.5.1.013, but it has me concerned that the project may not be using all of the files from the latest EPA release (5.1.013) as its base.

I don't like creating 3 new functions for saving a hotstart file via the swmm_saveHotstart function. Here's how I would have done it without needing to change anything in the original SWMM code:

int DLLEXPORT swmm_saveHotstart(char *hsfile)
{
// Sanity checks go here
    else
    {
        TFile tmpHotstart1 = Fhotstart1;
        TFile tmpHotstart2 = Fhotstart2;
        Fhotstart1.mode = NO_FILE;
        Fhotstart2.mode = SAVE_FILE;
        sstrncpy(Fhotstart2.name, hsfile, MAXFNAME); 
        if (hotstart_open()) hotstart_close();
        Fhotstart2 = tmpHotstart2;
        Fhotstart1 = tmpHotstart1;
    }
    return error_getCode(error_code_index);
}

@bemcdonnell I am trying to think of reasons why the implementation suggested by @LRossman will not be ideal but can't think of any major ones save for I am generally apprehensive about using global variables and repurposing one like this makes me worry even more. However, it is less intrusive and accomplishes the same task. What do you think ?

@LRossman, I like the simplicity and elegance of your suggested implementation. I wonder if perhaps we could consider a blended solution that could perhaps be more congruent with longer-term planning for this project (echoing @calebbuahin). I could be being way too presumptuous here but what if we think about our end-goal as a place where we encapsulate all of the globals and separate concerns better? By doing so, we can write code that is easily unit-testable. I think opening up an issue to talk about more general roadmapping philosophy would be worth all of our time! So you be the judge in this thread - Am I being presumptuous or not with my proposed end-goal? :-)

What I like about @jsadler2's and my proposed implementation is that we are passing file pointers into a function as an argument instead of telling this function to use a global. To me, this makes the code a lot more readable. What I don't like about the proposed implementation is exactly what you are saying @LRossman - we are explicitly calling saveRouting and saveRunoff in toolkitAPI.c. Directly talking to those two functions outside of hotstart.c is NOT elegant. I think swmm_saveHotstart(char *flname) should call a function like hotstart_close(TFile flptr). One thing I really do not like is using temporary variables to hold and store state. It doesn't feel as clean to me - but this is totally a personal style thing. Passing file handles around is a nice step toward an encapsulation. Also it will make it easier to unit test.

What are your thoughts?

Oh and a quick response to what you found:

@bemcdonnell as I'm reviewing this PR I noticed that the hotstart.c file being modified seems to come from v.5.1.012 (or maybe 5.1.011) and not 5.1.013

This came from a dev version of 5.1.013. One of the last things that was done was comments were stripped away. Looks like I introduced the comment back it when i resolved the merge conflicts into Jeff's code. I promise this is the only thing that I brought back in ;-)

@bemcdonnell I thought that the focus of this project was to supply SWMM with a comprehensive API, only making changes to the core engine code when it was absolutely necessary, thus making it easier to keep in synch with the official EPA version of the code. Although I'm sure there are lots of places where refactoring might be called for, my response would be "if it ain't broke don't fix it".

That being said, I would endorse someone or group forking the code to another project (not just a branch) and re-architecting it from the ground up (not just applying piecemeal refactoring to the current code). It's been almost 15 years since SWMM 5 was first released so maybe the software development cycle has run its course.

One source of ideas for what the re-design might look like can be found in a white paper I wrote for the re-design of EPANET. EPANET and SWMM have a lot in common from a structural point of view so the ideas presented there may be relevant.

@LRossman, I've elevated your last comment to a new Issue in #254. I'm writing a response to it now!

@jackieff has been newly assigned to this since she sounded really excited to do it! Thanks @jackieff !!!

#393 has replaced this PR.