Java Map anti-pattern?

Question

Edit: I've gotten a couple of answers that say what I already said in the question. What I am really interested in is finding corroborating reference material.

---

I am looking at a code sample that more or less follows this pattern:

Map<String, List> getListsFromTheDB() {
  Map<String, List> lists = new HashMap<String, List>();

  //each list contains a different type of object
  lists.put("xList", queryForListOfXItems());
  lists.put("yList", queryForListOfYItems());

  return lists;
}

void updateLists() {
  Map<String, List> lists = getListsFromTheDB();
  doSomethingWith(lists.get("xList"));
  doSomethingWith(lists.get("yList"));
}

My feeling is that this is an anti-pattern. What the coder should have done is create a class which can be returned, like this:

class Result {
  private final List<X> xList;
  private final List<Y> yList;

  public Result(xList, yList) {
    this.xList = xList;
    this.yList = yList;
  }

  public List<X> getXList() { xList; }
  public List<Y> getYList() { return yList; }
}

This would be more type-safe, avoid over-generalizing a very specific problem, and be less prone to errors at runtime.

Can anyone point me to any authoritative reference material which specifies that you should avoid this kind of pattern? Or, alternately, if it's actually a good pattern, please give justification.

Answer 1

I don't have any authoritative material, but my gut feeling is that, unless there's something more complex going on in the real code, using a Map this way doesn't bother me. The Result class feels a bit like overkill, actually.

Answer 2

I think the point is the number of Lists is fixed. Since you ensure the code uses 2 lists, the map is a little bit over-generalizing.

So 'class Result' is better I think.

Answer 3

I agree with you. Obviously the guy was lazy and used a Map to avoid creating a new class. The side effect is that the code that need to use getListsFromTheDB() will be less readable, and, as you mentionned, more error-prone.

Of course, there is the alternative in which the caller creates the lists:

void fillFromTheDB(List<X> xList, List<Y> yList) {
  //each list contains a different type of object
  xList.addAll(queryForListOfXItems());
  yList.addAll(queryForListOfYItems());
}

void updateLists() {
  List<X> xList = new ArrayList<X>();
  List<Y> yList = new ArrayList<Y>();
  fillFromTheDB(xList, yList);
  doSomethingWith(xList);
  doSomethingWith(yList);
}

Answer 4

I say it depends on the context.

If you return a map, the caller has to know the 'magic' keys "xList" and "yList" to get the actual data out of the map. I mean magic as magic constants. (You could iterate over the map to find the magic keys, but that's just a trick.) By using the map you have actually hidden the data, making it harder to get what one wants (x- and yLists).

The magic constants do not have to be so magically. If "xList" and "yList" would be the table names in the database (or whatever external strings), then I would expect to get a mapping from table names to object lists. Someone might add/rename/delete tables. (Or, maybe prettier, I would like to be able to query per table like getListFromTheDB("xList");. )

In your code you got this method

queryForListOfXItems();

That does smell like hard coded xList and yList. That would thus (IMO) make a map a bad choice.