Terrible Code

[java]
request.setParameter("qualifySkus", getSkusRepository(d, cItem));
[/java]

1. “qualifySkus” is confusing. Is it an array/list/collection of “qualifiedSKUs” or a flag that’s a result of “qualifyingSkus” or….
2. “qualifySKus” should be a constant with a nice comment, not an in-line String.
3. The method getSkusRespository seems like it would return a catalog repository, doesn’t it? Instead it takes in a List of String SkuIds, loads up the corresponding SKU RepositoryItems, removes any that have the property “isLive” set to false, and removes any that have a current inventory stock level of zero. It then returns an ArrayList of those filtered SKU RepositoryItems. Perhaps a better name might be “getLiveInStockSKUs”?
4. What on earth is “d”? Even looking at the full code of this class, it’s very difficult to tell what d is meant to contain. It’s actually a List of Strings of SkuIds that are qualifying skus for a given promo. “qualifiedSkus” would be a better name.
5. cItem is a commerce item. However it’s not actually used by the getSkusRepository method at all. There’s no reason to pass it in.
6. This line is in an ATG droplet and shoves the result of the getSkusRepository method into a request param before servicing an oparam. However, as you can see, it doesn’t inspect the output of the method. As I explained above, the method actually filters a list of SKUs based on isLive and current inventory state. It’s very possible that there will be no live and in-stock SKUs, and the param’s value will be null or an empty list. In that case, we’d actually want to render a different oparam, which is defined and called elsewhere, but not here. Validate your output!

That’s six issues in one line. Please don’t write code like this.