CodeSOD: Bop It

Over twenty years ago, Matt's employer started a project to replace a legacy system. Like a lot of legacy systems, no one actually knew exactly what it did. "Just read the code," is a wonderful sentiment, but a less practical solution when you've got hundreds of thousands of lines of code and no subject-matter experts to explain it, and no one is actually sure what the requirements of the system even are at this point.

There's a standard practice for dealing with these situations. I'm not sure it should be called a "best practice", but a standard one: run both systems at the same time, feed them the same inputs and make sure they generate the same outputs.

We cut to present day, when the legacy system is still running, and the "new" system is still getting the kinks worked out. They've been running in parallel for twenty years, and may be running in that state for much much longer.

Matt shares some C code to illustrate why that might be:

 while (i < *p_rows) { switch (she_bop.pair_number[i]) { case -5: sell_from[j+4]=she_bop.from[i]; sell_price[j+4]=she_bop.price[i]; sell_bid[j+4]=she_bop.bid[i]; break; case -4: sell_from[j+3]=she_bop.from[i]; sell_price[j+3]=she_bop.price[i]; sell_bid[j+3]=she_bop.bid[i]; break; case -3: sell_from[j+2]=she_bop.from[i]; sell_price[j+2]=she_bop.price[i]; sell_bid[j+2]=she_bop.bid[i]; break; case -2: sell_from[j+1]=she_bop.from[i]; sell_price[j+1]=she_bop.price[i]; sell_bid[j+1]=she_bop.bid[i]; break; case -1: sell_from[j+0]=she_bop.from[i]; sell_price[j+0]=she_bop.price[i]; sell_bid[j+0]=she_bop.bid[i]; break; case +1: buy_from[j+0]=she_bop.from[i]; buy_price[j+0]=she_bop.price[i]; buy_bid[j+0]=she_bop.bid[i]; break; case +2: buy_from[j+1]=she_bop.from[i]; buy_price[j+1]=she_bop.price[i]; buy_bid[j+1]=she_bop.bid[i]; break; case +3: buy_from[j+2]=she_bop.from[i]; buy_price[j+2]=she_bop.price[i]; buy_bid[j+2]=she_bop.bid[i]; break; case +4: buy_from[j+3]=she_bop.from[i]; buy_price[j+3]=she_bop.price[i]; buy_bid[j+3]=she_bop.bid[i]; break; case +5: buy_from[j+4]=she_bop.from[i]; buy_price[j+4]=she_bop.price[i]; buy_bid[j+4]=she_bop.bid[i]; break; default: she_bop_debug(SHE_BOP_DBG, SHE_DBG_LEVEL_3, "duh"); break; } i++; }

Here, we have a for-case antipattern that somehow manages to be wrong in an entirely different way than the typical for-case pattern. Here, we do the same thing regardless of the value, we just change our behavior based on a numerical offset. That offset, of course, can easily be calculated based on the she_bop.pair_number value.

That said, there are other whiffs of ugliness that we can't even see here. Why the three arrays for buy_from, buy_price, and buy_bid, when they clearly know how to use structs. Then again, do they, as she_bop seems to be a struct of arrays instead of a more typical array of structs. And just what is the relationship between i and j anyway?

And then, of course, there's the weird relationship with that pair number- why is it in a range from -5 to +5? Why do we log out a debugging message if it's not? Why is that message absolutely useless?

More code might give us more context, but I suspect it won't. I suspect there's a very good reason this project hasn't yet successfully replaced the legacy system.

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