Refactoring dwflow.c

[LRossman]

A hallmark of readable and maintainable code are functions that are small and do one thing (see Chapter 3 in the classic "Clean Code" book by Robert C. Martin). I think SWMM mostly adheres to these concepts with some exceptions. One of these is the dwflow_findConduitFlow function in dwflow.c. It evaluates the combined continuity/momentum equation to find a new conduit flow estimate under dynamic wave flow routing and consists of 236 lines of code. I recently refactored this function to look as follows:

void  dwflow_findConduitFlow(int j, int steps, double omega, double dt)
//
//  Input:   j        = link index
//           steps    = number of iterative steps already taken
//           omega    = under-relaxation parameter
//           dt       = time step (sec)
//  Output:  new conduit flow value (cfs)
//  Purpose: updates flow in conduit link by solving finite difference
//           form of combined continuity and momentum equations.
//
{
    TData d;            // conduit data struct storing intermediate results
    double q;           // conduit flow at the end of the current time step
    double sigma;       // inertial damping factor

    initConduitData(j, &d);
    findFlowDepths(j, &d);
    findSurfaceAreas(j, &d);
    findFlowAreas(j, &d);
    if (conduitIsDry(j, dt, &d)) return;
    sigma = findInertialDamping(j, &d);
    q = findNewFlow(j, sigma, dt, &d);
    q = findFlowLimitation(j, steps, omega, q, &d);
    saveNewValues(j, q, &d);
}

This revised code calls a number of new subordinate functions, containing code off-loaded from the original function, that make it easier to follow what dwflow_findConduitFlow is actually doing.

Testing shows that the refactored file produces the same results as the original. One concern is if the additional function calls and use of the TData struct to share intermediate results between them would add significant execution time. I ran timing tests on two rather large data sets. The results are shown in the table below:

Data Set	# Subcatchments	# Links	Run Duration	Original Function	Refactored Function
1	1345	2307	24 hrs	136 sec	140 sec
2	1969	8683	24 hrs	655 sec	647 sec

It appears that the refactored function doesn't result in any significant additional execution time. I'd be interested to know if people think this code revision (made only for readability and maintainability) is worth inserting into the project.

[michaeltryby]

Hey @LRossman, Looks like a worthwhile refactoring effort. @cbuahin and I are both interested in seeing what you have done. I think it definitely has a place in the project. Thanks!

[dickinsonre]

any change that makes it "easier to follow what dwflow_findConduitFlow is actually doing" without a time penalty should be welcomed.

[LRossman]

I have attached the refactored dwflow.c file so people can view it and comment. It's been written to comply with the draft SWMM 5 Style Guide posted in Discussion #130.
dwflow-refactored5.txt

[michaeltryby]

@LRossman Fantastic! I’ll have a look see!

[jlhomme]

It's definitively an improvement 👍