CodeSOD: Eine Kleine ProductListItems

Art received a job offer that had some generous terms, and during the interview process, there was an ominous sense that the hiring team was absolutely desperate for someone who had done anything software related.

Upon joining the team, Art found out why. Two years ago, someone had decided they needed to create a web-based storefront, and in a fit of NIH syndrome, it needed to be built from scratch. Unfortunately, they didn't have anyone working at the company with a web development background or even a software development background, so they just threw a book on JavaScript at the network admin and hoped for the best.

Two years on, and they didn't have a working storefront. But they did have code like this:

productListItems= function(zweier,controll){ var cartProductListItems = []; if (zweier=="zweier"){ controll.destroyItems(); for (var y = 0; y <= 999; y=y+2) { controll.addItem(new sap.ui.core.ListItem({ text: y+2, key: y+2 })); } controll.setSelectedKey(2) } else if(zweier=="einer"){ controll.destroyItems(); for (var _i = 0; _i <= 999; _i++) { controll.addItem(new sap.ui.core.ListItem({ text: _i+1, key: _i+1 })); } controll.setSelectedKey(1) } else{ for (var _i = 0; _i <= 999; _i++) { cartProductListItems.push(new sap.ui.core.ListItem({ text: _i+1, key: _i+1 })); } } return cartProductListItems; };

controll is the on-screen control we're populating with list items. zweier controls the skip pattern- if it's "zweier", then skip by two. If it's "einer", skip by one, and if it's neither, populate an array. Return the array, populated or otherwise, at the end.

Now, someone didn't like that function, so they implemented an alternative which takes more parameters:

productListItems2= function(zweier,index,response,model){ var cartProductListItems = []; if (model!="" && model!=undefined ){ var data = response.oModel.getProperty(response.sPath) var mindestbestellmenge = data.BOMRABATT; if (mindestbestellmenge!="" && mindestbestellmenge != undefined){ if (mindestbestellmenge.toString().length == 1){ //do nothing }else{ mindestbestellmenge=mindestbestellmenge.split(".")[0] } } if (mindestbestellmenge!="1" && mindestbestellmenge!="0" && mindestbestellmenge != undefined && data.VERPACKUNGSEINHEIT=="ZS"){ var mindestbestellmenge = parseInt(mindestbestellmenge); cartProductListItems.push(new sap.ui.core.ListItem({ text: mindestbestellmenge, key: mindestbestellmenge })); }else{ cartProductListItems.push(new sap.ui.core.ListItem({ text: 1, key: 1 })); } return cartProductListItems } else{ if (zweier=="zweier"){ cartProductListItems.push(new sap.ui.core.ListItem({ text: 2, key: 2 })); } else if(zweier=="einer"){ cartProductListItems.push(new sap.ui.core.ListItem({ text: 1, key: 1 })); } else{ cartProductListItems.push(new sap.ui.core.ListItem({ text: 1, key: 1 })); } } return cartProductListItems; };

Okay, once again, we do some weird munging to populate a list, but we still have this bizarre zweier variable. Which, by the way, ein is one, zwei is two, so they're obviously using the spelled out version of a number instead of the actual number, and I could do with a little g'suffa at this point.

But you know what? This wasn't enough. They had to add another version of productListItems.

productListItems4 = function( control, min, max, step ){ step = +step || 1; min = +min ? +min + ( +min % +step ) : +step; max = +max || 999; var items = []; var ListItem = sap.ui.core.ListItem; var i; for ( i = min; i <= max; i += step ) items.push( new ListItem({ text: i, key: i }) ); if ( control ) { control.removeAllItems(); items.forEach( control.addItem.bind( control ) ); control.setSelectedKey( min ); } return items;};

This one is kinda okay. I don't love it, but it just about makes sense. But wait a second, why is it productListItems4? What happened to 3?

productListItems3= function(oControll, sLosgroesse){ var anzahl = window.anzahl; var cartProductListItems = []; if(oControll){ if(sLosgroesse>1 || anzahl>1){ if (sLosgroesse>1){ oControll.destroyItems(); for (var y = 1; y <= 999; y++) { oControll.addItem(new sap.ui.core.ListItem({ text: y*sLosgroesse, key: y*sLosgroesse })); } oControll.setSelectedKey( sLosgroesse || anzahl || 1 );// oControll.setSelectedKey(2) } if(anzahl) if(anzahl>1){ oControll.destroyItems(); for (var y = 1; y <= 999; y++) { oControll.addItem(new sap.ui.core.ListItem({ text: y*anzahl, key: y*anzahl })); } oControll.setSelectedKey( sLosgroesse || anzahl || 1 ); // oControll.setSelectedKey(2) } } else{ oControll.destroyItems(); for (var y = 1; y <= 999; y++) { oControll.addItem(new sap.ui.core.ListItem({ text: y, key: y })); } } } else{ if(sLosgroesse>1){ for (var _i = 1; _i <= 999; _i++) { cartProductListItems.push(new sap.ui.core.ListItem({ text: _i*sLosgroesse, key: _i*sLosgroesse })); } } else{ for (var _i = 1; _i <= 999; _i++) { cartProductListItems.push(new sap.ui.core.ListItem({ text: _i, key: _i })); } } } return cartProductListItems; };

Oh. There it is. I'm sorry I asked. Nice use of the anzahl global variable. Some global variables are exactly what this needed. In this case, it holds the skip number again, so a riff on einer and zweier but without spelling things out.

A quick search for calls to any variation of productListItems shows that there are 2,000 different invocations to one of these methods, and a sampling shows that it's a little of all of them, depending on the relative age of a given code module.

[Advertisement] Ensure your software is built only once and then deployed consistently across environments, by packaging your applications and components. Learn how today!