Markus does QA, and this means writing automated tests which wrap around the code written by developers. Mostly this is a "black box" situation, where Markus doesn't look at the code, and instead goes by the interface and the requirements. Sometimes, though, he does look at the code, and wishes he hadn't.

Today's snippet comes from a program which is meant to generate PDF files and then, optionally, email them. There are a few methods we're going to look at, because they invested a surprising amount of code into doing this the wrong way.

protected override void Execute()
{
	int sendMail = this.VerifyParameterValue(ParamSendMail);

	if (sendMail == -1)
		return;

	if (sendMail == 1)
		mail = true;

	this.TraceOutput(Properties.Resources.textGetCustomerForAccountStatement);
	IList<CustomerModel> customers = AccountStatement.GetCustomersForAccountStatement();
	if (customers.Count == 0) return;

	StreamWriter streamWriter = null;
	if (mail)
		streamWriter = AccountStatement.CreateAccountStatementLogFile();

	CreateAccountStatementDocumentEngine engine = new CreateAccountStatementDocumentEngine();

	foreach (CustomerModel customer in customers)
	{
		this.TraceOutput(Properties.Resources.textCustomerAccountStatementBegin + customer.DisplayName.ToString());

		// Generate the PDF, optionally send an email with the document attached
		engine.Execute(customer, mail);

		if (mail)
		{
			AccountStatement.WriteToLogFile(customer, streamWriter);
			this.TraceOutput(Properties.Resources.textLogWriting);
		}
	}
	engine.Dispose();
	if (streamWriter != null)
		streamWriter.Close();
}

Now, this might sound unfair, but right off the bat I'm going to complain about separation of concerns. This function both generates output and emails it (optionally), while handling all of the stream management. Honestly, I think if the developer were simply forced to go back and make this a set of small, cohesive methods, most of the WTFs would vanish. But there's more to say here.

Specifically, let's look at the first few lines, where we VerifyParameterValue. Note that this function clearly returns -1 when it fails, which is a very C-programmer-forced-to-do-OO idiom. But let's look at that method.

private int VerifyParameterValue(string name)
{
	string stringValue = this.GetParam(name, string.Empty);

	bool isValid = this.VerifyByParameterFormat(name, stringValue);

	if (!isValid)
		return -1;

	int value = -1;

	try
	{
		value = Convert.ToInt32(stringValue);
	}
	catch (Exception e)
	{
		this.TraceOutput(string.Concat("\n\n\n", e.Message, "\n\n\n"));

		return -1;
	}
	return value;
}

We'll come back to the VerifyByParameterFormat but otherwise, this is basically a wrapper around Convert.ToInt32, and could easily be replaced with Int32.TryParse.

Bonus points for spamming the log output with loads of newlines.

Okay, but what is the VerifyByParameterFormat doing?

private bool VerifyByParameterFormat(string name, string value)
{
	string parameter = string.Empty;
	string message = string.Empty;
	
	if (value.Length != 1)
	{
		parameter = Properties.Resources.textSendMail;
		message = string.Format(Properties.Resources.textSendMailNotValid, value);

		this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n"));

		return false;
	}

	string numbers = "0123456789";
	char[] characters = value.ToCharArray();

	for (byte i = 0; i < characters.Length; i++)
	{
		if (numbers.IndexOf(characters[i]) < 0)
		{
			parameter = Properties.Resources.textSendMail;
			message = Properties.Resources.textSendMailNotValid;
			
			this.TraceOutput(string.Concat("\n\n\n", message, "\n\n\n"));
			return false;
		}
	}
	return true;
}

Oh, it just goes character by character to verify whether or not this is made up of only digits. Which, by the way, means the CLI argument needs to be an integer, and only when that integer is 1 do we send emails. It's a boolean, but worse.

Let's assume, however, that passing numbers is required by the specification. Still, Markus has thoughts:

Handling this command line argument might seem obvious enough. I'd probably do something along the lines of "if (arg == "1") { sendMail = true } else if (arg != "0") { tell the user they're an idiot }. Of course, I'm not a professional programmer, so my solution is way too simple as the attached piece of code will show you.

There are better ways to do it, Markus, but as you've shown us, there are definitely worse ways.

[Advertisement] Plan Your .NET 9 Migration with Confidence
Your journey to .NET 9 is more than just one decision.Avoid migration migraines with the advice in this free guide. Download Free Guide Now!