Terrible Code
[java]
request.setParameter("qualifySkus", getSkusRepository(d, cItem));
[/java]
- “qualifySkus” is confusing. Is it an array/list/collection of “qualifiedSKUs” or a flag that’s a result of “qualifyingSkus” or….
- “qualifySKus” should be a constant with a nice comment, not an in-line String.
- 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”?
- 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.
- 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.
- 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.
