CodeSOD: Drain the Swamp

You may remember Virginia N from An Extinction Event, where she struggles to refactor a legacy project with some" unusual design principles. ReSharper still continues to choke to death on their codebase, but her management has let her know, this won't be a problem going forward.

"You see," her boss explained, "we're going to move the logic into stored procedures. That way, we can more easily re-use the logic between the Windows Forms client and the Web app."

"Oh, there's going to be a web app, now?" Virginia asked.

"Yes! They're going to use best practices, like unit testing, so that they don't end up with the same kind of mess we have," her boss said.

Virginia was halfway to filing a request for a transfer when she heard more about the web project. Then she heard about their plan. They weren't going to simply build a web-client for their backend, but instead were going to build an inner platform. The page logic would be a simple template, and all of the rules, styling, data and display logic would be stored as data in the database. "It's gonna be really flexible!"

Virginia decided to stick with the fetid field she knew, instead of the "green field" which was going to be a fetid swamp in a matter of weeks.

In Virginia's swamp, she has many, many 40,000 line classes. That much code means the code has no real cohesion, so it leads to Virginia finding variables named thus:

public bool IReallyDontWantToFetchTheDataOnLoadButDontWantToChangeTheOtherVariablesCauseWhoKnowsWhatWillHappen=false;

It's at least descriptive. It also exists in the class side-by-side with variables like blnGetData, GetDataOnFormLoad, and GetDataOnFormrLoadS.

Elsewhere in the same file, they have a different problem. They added a control to the view, and needed an accessor method to decide whether it was visible or not. Actually, they needed a few, and they knew that the form would be changing over time, so they needed something that was "dynamic".

Now, they could have simply added properties and getters/setters as the form changed and dealt with the follow on changes, but someone decided it was time to stress "Closed for Modification" was a good object-oriented practice. They left out the "Open for Extension" part of the open/closed principle, so they used this code instead, which uses a few index properties to decide which control should be modified.

private void SetVisiblePicNdt(){ string NomBtnNDT=""; string NomBtnChant=""; if (m_IndexChant>0) { NomBtnNDT="btn"+(m_IndexChant+1).ToString(); NomBtnChant="btn"+(m_IndexChant).ToString(); } else { NomBtnNDT="btn"+(m_IndexLB+1).ToString(); } Control sectMain=null; for (int i=0;i<ctlRecherche.Controls.Count;i++) { if (ctlRecherche.Controls[i].Name=="sectMain") { sectMain=ctlRecherche.Controls[i]; break; } } if (sectMain!=null) { for (int i=0;i<sectMain.Controls.Count;i++) { if (sectMain.Controls[i].Name=="panFilterSection") { panFilterSection=sectMain.Controls[i]; break; } } } if (panFilterSection!=null) { for (int i=0;i<panFilterSection.Controls.Count;i++) { if (panFilterSection.Controls[i].Name==NomBtnNDT) { panFilterSection.Controls[i].Visible=false; break; } } }}

Virginia has many more horrors to share, so expect more examples from this code base.

[Advertisement] Release!is a light card game about software and the people who make it. Play with 2-5 people, or up to 10 with two copies - only $9.95 shipped!