Method has same erasure: How should I go about refactoring? [closed]

Question

Currently, in a utility class I have methods like so:

public static String getNextAvailableNumber(List<MyObject> currentObjects)
{
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(currentObjects, null);
    return UtilClass.formatNumber(nextNumber); //formats number as string
}

public static int getNextAvailableNumberAsInteger(List<MyObject> currentObjects, List<String> otherUnavailableNumbers)
{
    //returns nextNumber as int.
}

I now find myself wanting to add another method...

public static String getNextAvailableNumber(List<String> unavailableNumbers)
{
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(null, unavailableNumbers);
    return UtilClass.formatNumber(nextNumber);
}

You may notice that I would end up with a List argument in both places, causing a method has same erasure compiler error.

I'm considering the following solution..

//replace old getNextAvailableNumber() with generic version
public static String getNextAvailableNumber(List<?> objects)
{
    List<MyObject> objectList = new ArrayList<MyObject>();
    List<String> stringList = new ArrayList<String>();

    if (objects.get(0) instanceof MyObject)
    {
        objectList = (List<MyObject>) objects;
    }
    else if (objects.get(0) instanceof String)
    {
        stringList = (List<String>) objects;
    }
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(objectList, stringList);
    return UtilClass.formatNumber(nextNumber);
}

From what I've read, using instanceof is generally a sign that something can be done in a better way. Is there a better way? How should I go about refactoring this?

Answer 1

I strongly suspect that you have the wrong problem -- this feels like UtilClass.getNextAvailableNumberAsInteger is implemented incorrectly. It's also not clear why you want to use the same method name when the argument has a completely different meaning.

That said... my recommendation would be to create a new function object.

class NumberGenerator {
    private List<MyObject> objects = null;
    private List<String> unavailableNumbers = null ;

    public NumberGenerator withObjects(List<MyObject> objects) {
        this.objects = objects;
        return this;
    }

    public NumberGenerator withUnavailableNumbers(List<String> unavailableNumbers) {
        this.unavailableNumbers = unavailableNumbers;
        return this;
    }

    public int getNext() {
        return UtilClass.getNextAvailableNumberAsInteger(objects, unavailableNumbers);
    }

    // OPTIONAL
    public static NumberGenerator newInstance () {
        return new NumberGenerator();
    }
}

Then your clients can use the NumberGenerator to get what they need, without having to worry about the details

int nextNumber = NumberGenerator.newInstance().withUnableNumbers(otherNumbers).getNext();

Other comments: it looks really odd that your "other unavailable numbers" are stored in a collection of String. It looks somewhat odd that you are using a collection of MyObject to produce integers as well. Why do the collections need to be List, as opposed to Collection or Iterable?

    if (objects.get(0) instanceof MyObject)

That's an awful idea; you're rooked if the array you are passed is empty. If it's true that an empty list means you don't care what kind of list it is, that very strongly suggests that the entire interface is broken.

List<MyObject> objectList = new ArrayList<MyObject>();

If the list isn't supposed to by modified by the function, then you might be better off using Collections.EMPTY_LIST; it would help clarify your intent.

Answer 2

If you want the getNextAvailableNumber method to take a List of any type, then you should use a template parameter, like this:

public static <T> String getNextAvailableNumber(List<T> currentObjects) {
    int nextNumber = UtilClass.getNextAvailableNumberAsInteger(currentObjects, null);
    return UtilClass.formatNumber(nextNumber);
}

public static <T> int getNextAvailableNumberAsInteger(List<T> currentObjects, List<String> otherUnavailableNumbers) {
    // returns nextNumber as int.
}

Since you didn't give much context, it's a bit hard to advise more on this code. There are few clearly bad signs though:

• objects.get(0) instanceof MyObject : using instanceof to distinguish on a list's type (not to mention, without checking if the list has any elements) is always a bad practice. It would be very hard to find a legitimate use case for this.

• (List<MyObject>) objects; : casting to a list is almost always a bad practice, and the compiler will give you a warning which you shouldn't ignore. Legitimate use cases are extremely rare, and this is not one of them.

• Passing null for list values is not a good practice. It's better to avoid and use Collections.emptyList() instead. In most practical situations, avoiding null values leads to safer and more ergonomic implementations.

Answer 3

public static String getNextAvailableNumber(List<?> objects)

Just don't do it. Unless you're ready to accept List<MaaartinsTypeNo123>, too. If you're not, just use two differently named methods. You need to invent the names and type a bit more, but not losing type safety is worth it.

---

If you still feel like willing to accept List<?>, then better check each element you work with. Something like

private int anythingToInt(Object o) {
    if (o == null) {
        throw new NullPointerException();
    } else if (o instanceof Integer) {
        return ((Integer) o).intValue();
    } else if (o instanceof String) {
        return Integer.parseInt((String) o);
    }
    .................
    throw new IllegalArgumentException("Expected Integer, or String, or ..., got " + o.getClass());
}

and use it in something like

for (Object o : objects) {
    int n = anythingToInt(o);
    if (isAvailable(n)) {
        return n;
    }
}

But better don't do it. Maybe just convert the input list to what you really want ASAP?