CodeSOD: Maintaining Yourself

When moving from one programming language to another, it's easy to slip into idioms that might be appropriate in one, but are wildly out of place in another. Tammy's company has some veteran developers who spent most of their careers developing in COBOL, and now' they're developing in C#. As sufficiently determined programmers, they're finding ways to write COBOL in C#. Bad COBOL.

Methods tend towards long- one thousand lines of code isn't unusual. Longer methods exist. Every method starts with a big long pile of variable declarations. Objects are used more as namespaces than anything relating to object-oriented design. So much data is passed around as 2D arrays, because someone liked working with data like it lived in a table.

But this method comes from one specific developer, who had been with the company for 25+ years. Tammy noticed it because, well, it's short, and doesn't have much by way of giant piles of variables. It's also, in every way, wrong.

public static bool _readOnlyMode;
public static bool _hasDataChanged;

//...

public bool HasDataChanged()
{
	try {
		if (_readOnlyMode == false) {
			_hasDataChanged = true;
			
			btnPrint.Enabled = true;
			btnPrintPreview.Enabled = true;
			btnSave.Enabled = true;
			btnCancel.Enabled = true;
		}
	} catch (Exception ex) {
	}
}

So, first, we note that _reandOnlyMode and _hasDataChanged are static, reinforcing the idea that this class isn't an object, but it's a namespace for data. Except the method, HasDataChanged isn't static, which is going to be fun for everyone else trying to trace through how data and values change. Note, also, that these names use the _ prefix, a convention used to identify private variables, but these are explicitly public.

The method is marked as a bool function, but never returns a value. The name and signature imply that this is a simple "question" method: has the data changed? True or false. But that's not what this method does at all. It checks if _readOnlyMode is false, and if it is, we enable a bunch of controls and set _hasDataChanged to true. Instead of returning a value, we are just setting the global public static variable _hasDataChanged.

And the whole thing is wrapped up in an exception handler that ignores the exception. Which, either none of this code could ever throw an exception, making that pointless, or there's no guarantee that all the btn… objects actually exist and there's a chance of null reference exceptions.

Tammy has inherited this application, and her assignment is to perform "maintenance". This means she doesn't touch the code until something breaks, then she spends a few days trying to replicate the error and then pick apart the thicket of global variables and spaghetti code to understand what went wrong, then usually instead of changing the code, she provides the end user with instructions on how to enter that data in a way that won't cause the application to crash, or manually updates a database record that's causing the crash. "Maintenance" means "keep in the same way", not "fix anything".

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

This post originally appeared on The Daily WTF.