CodeSOD: Built Up

In most languages, strings are immutable. As developers, we often need to manipulate strings- for example, constructing output through concatenation.

Constructs like foo += " and then I appended this"; "solve" this immutability issue by creating a new string instance. If you're doing a long round of concatenation, especially if it happens inside of a loop, this could get very expensive, which is why most languages also have a StringBuilder type, which allows you to append without all that overhead of new instances. Often, the advice is that you should prefer StringBuilder objects to string.

Jonathan's co-worker applied this advice without understanding why.

private static string PrivateValidateRequestAndGetReserve(string ProductCode, int TransactionType, string Username, string Password, ref string ReserveId){ StringBuilder ReturnMessage = new StringBuilder(); string TransactionCode = Enum.GetName(typeof(Common.Enums.TransactionCodesEnum), TransactionType); ReserveId = string.Empty; using (var AdminWS = new wsAdmin.AdminClient()) { if (!AdminWS.AuthenticateUser(Username, Password)) { ReturnMessage.Append("Error Logging on with the username and password supplied."); return ReturnMessage.ToString(); } if (!AdminWS.CanUserAccessProductAndTransaction(Username, ProductCode, TransactionCode)) { ReturnMessage.Append("You don't have access to this transaction."); return ReturnMessage.ToString(); } if (!(AdminWS.CheckBalanceForTransactionFromUsername(out ReserveId, Username, TransactionCode))) { ReturnMessage.Append("Not Enough Credits to perform current transaction"); return ReturnMessage.ToString(); } } return ReturnMessage.ToString();}

This isn't the worst of it. First, note all those calls to AdminWS methods. The developers who wrote those did not believe in throwing exceptions, since an uncaught exception could cause the program to crash. Instead, they wrote every method to return a boolean value indicating success or failure. This meant if any function needed to return a value, that could only be done as an out or ref parameter.

But the real prize here is with the parameter int TransactionType. As you can see in the code, they convert the int into a string by pulling it through an enumerated type called TransactionCodesEnum. It makes you wonder, why is TransactionType an int, couldn't they have just passed the enum into the method? There must be a good reason, right? Well, here's the code that calls this method:

Enums.TransactionCodesEnum transactionEnum = getCurrentTransactionCode();string validationText = Admin.ValidateRequestAndGetReserve(ProductName, (int) transactionEnum, username, password, ref reserveId);

[Advertisement] Manage IT infrastructure as code across all environments with Puppet. Puppet Enterprise now offers more control and insight, with role-based access control, activity logging and all-new Puppet Apps. Start your free trial today!