CodeSOD: Prefixual

Maciek has the distinct pleasure of working on Dynamics Ax, and ERP system. Like every other ERP system, it’s endlessly customizable, and scriptable. In this case, scriptable in a custom language called “X++”.

While it’s probably entirely possible to write good code under these circumstances, it’s not an environment conducive to that. And that’s how Maciek inherited this method:

public boolean modified()
{
boolean ret;
str itemPrefix;

str itemId;
ResponseCodeTable responseCodeTable;
;

itemId = SalesLine_ItemId.getLine(1);
if (itemId)
{
itemPrefix = strupr(strkeep(substr(itemId, 1, 2),“qwertyuiopasdfghjklzxcvbnm”));

responseCodeTable = ResponseCodeTable::existItemPrefix(itemPrefix);
if(responseCodeTable)
{
itemId = substr(itemId, strlen(itemPrefix)+1, strlen(itemId) – strlen(itemPrefix));
if (InventTable::exist(itemId))
{
this.pasteText(itemId, false);
SalesLine.PrefixItem = itemPrefix;
if(!ResponseCodeTable::findById(salesTable.ResponseCodeId).SuppressLineLevelPricing)
SalesLine.ResponseCodeId = responseCodeTable.ResponseCodeId;
}
}
}

ret = super();

return ret;
}

Now, this is a function called modified which returns a boolean value. This leads me to believe that this function is checking if a record is dirty- it returns true if the record has been modified and not saved to the database.

That is not what it does.

We start by getting an item ID from the first line of the SalesLine_ItemId. If it has one, we then extract its prefix- we use substr to grab two characters, filter the characters to just alphabetic characters, and convert to upper case. The purpose here is to validate the expected format of our item prefix- two alphabetic characters. If the item ID isn’t in the correct format, we’ll get something other than two characters in our itemPrefix variable.

Then we call ResponseCodeTable::existItemPrefix, using that itemPrefix. You may assume that this is just doing an existence check- but note the variable we’re stuffing the return in. It’s type is ResponseCodeTable- this does not check for existence, but actually queries the database for full records.

If there is a record, we do some string munging, call a different exist method (for inventory records?), and then populate a few fields on our SalesLine.

Finally, we call super() and return its value- which probably is doing what we expected the function to do in the first place, which is check if the record is dirty or not.

The irony here, is that because we’re manipulating the record under examination, we might have made it dirty just by validating it. I think. I don’t know, because it’s impossible to understand this function in isolation. It’s far off from the single responsibility principle, and instead is doing multiple things. This is likely because we needed to hook this functionality in some where, and based on the lifecycle of data in Dynamics Ax (or these scripting extensions), this looked like the most convenient place to do it.

I’ve seen this kind of code a lot. I’ve written a fair bit of this kind of code: you need a new feature, there’s no good place to hook the functionality in, so you find a place that touches the right part of the lifecycle and just wedge this into there. Voila, feature works, release it, and users are happy.

Technical debt becomes a problem for future you, or more likely, your successor.

A special shoutout to how they list off their alphabet by just following the QWERTY keyboard layout. That, itself, is the real WTF, as we all know the correct layout should have been “pyfgcrlaoeuidhtnsqjkxbmwvz”.

Source: Read More