Checking for existence of object by id, name, display name, or web id

Question

I have the following code:

public final boolean doesExistById(Long id) {
    return dataAccessObject.findById(id) != null;
}

public final boolean doesExistByName(String name) {
    return dataAccessObject.findByName(name) != null;
}

public final boolean doesExistByDisplayName(String displayName) {
    return dataAccessObject.findByDisplayName(displayName) != null;
}

public final boolean doesExistByWebId(String webId) {
    return dataAccessObject.findByWebId(webId) != null;
}

My Product class has properties id, name, displayName, wedId.
dataAccessObject.findBy____() returns an object of type Product, if it can be found in the data store, or null if it cannot.

I would like to reduce this chunk of code, if possible, because I have many objects that require the doesExist() pattern as above. The client code will only know one of these properties.

---

A possible solution I thought of would be to do this:

public final boolean doesExist(Long id, String name, String displayName, String webId) {..}

and then call it with null for unknown fields while using if statements to determine which field has a value. But is there another way that is more elegant?

---

One of the answers I got over was this, from user StriplingWarrior:

You are recognizing that the "does exist" part of all of these methods is exactly the same, and that makes you want to avoid repeating it, but the "ByXxx" part of these methods is completely different.

What you've got is far better than what you're thinking of doing. Please don't change your method signature to require callers to provide null values for all but one argument. That is highly error-prone, as it provides no compile-time errors for a variety of different ways that people might use the method signature incorrectly.

One thing you might want to consider doing is separating the "does exist" piece of this into its own mechanism:

public final Optional<MyObj> byId(Long id) {
    return Optional.ofNullable(dataAccessObject.findById(id));
}

So instead of saying service.doesExistById(123), you'd say service.byId(123).isPresent(). This represents the same meaning semantically, but it's broken into separate parts, which means you can reuse byId(), byName(), etc., for other purposes where you need the actual object, and not just to know whether it exists.

My ultimate question is, aside from the current solution, is there another way to refactor this code block so I don't have so much repeated similar code?

Answer 1

To me such a method

public final boolean doesExistById(Long id) {
    return dataAccessObject.findById(id) != null;
}

is already a code smell. What do you expect it to be used for. Something like

if (doesExistById(id)) {
    return getById(id);
} else {
    return new Something(id);
}

which means you're doing the lookup twice in the first branch. This is not too bad as there's caching somewhere, but it's strange. So I'd prefer to have no such method.

---

The other thing is that all it does id delegating to the dataAccessObject. Can't you use it directly. It looks like instead of calling

 dataAccessObject.findById(id)

you call

 somethingContainingTheDataAccessObject.findById(id)

Does it make sense? It might, but without more code we can't tell.

---

It looks like a typical XY problem. You're asking how to improve X (the repetitive doesExist* while having a problem Y (kept secret).

So all we can do is to tell you what not to do.

---

public final boolean doesExist(Long id, String name, String displayName, String webId) {..}

This could be a good idea if you needed a combined search often. But you're speaking about many methods and converting them into one method with many arguments is a step obviously in the wrong direction. It's much longer on the call site, much more error-prone and slightly less efficient. No gain.

---

public final Optional<MyObj> byId(Long id) {
    return Optional.ofNullable(dataAccessObject.findById(id));
}

My favorite rant says it all, though in this case it mightn't be that bad. You could use something like

public final Something getOrCreateById(Long id, Function<Long, Something> creator) {
    Something result = dataAccessObject.findById(id);
    return result != null ? result : creator.apply(id);
}

It wouldn't reduce the number of needed methods, but it could make them exactly what you need (remember, Y is kept secret).

With Java 8, it could get pretty compact.

Answer 2

Are you sure you need these methods at all? There's a level of abstraction which may exceed the needs of your actual use-case.

The context around your question is not complete, but your pattern is relatively common. There are three situations in which people write code like yours:

1. the DAO is not visible to the code that is calling doesExist****()
2. the code is a simple boolean check for existence (used in a conditional for something - the logical use-case you have here).
3. the code is in a conditional, but it needs the product anyway, e.g. if (doesExistById(12345)) { .... Product prod = getById(12345); ....}

If you are wrapping the DAO and do not want to expose it, then all you are doing here is introducing an abstraction that does no real work. I presume you already have exposed the actual "get" methods to retrieve the Product.

If you are going to need to return the Product anyway, then two calls to the wrapper to check if it exists, followed by a call to get it, is inefficient.

Bottom line, I would simply get rid of the methods entirely, and use a null/not-null check as needed. For example:

Product prod = dao.getByID(12345);
if (prod  != null) {
    prod.doSomething(....);
}

That way the code is still logical (the product exists), and is also efficient (only one call to the DAO). It is even better if you use functional extraction for the work:

public static boolean doSomethingWith(Product prod) {
    if (prod == null) {
        return false;
    }
    .....
    return true;
}

and then call it as you need with:

if (doSomethingWith(dao.getByID(12345))) {
    .....
}

You can even chain it using shortcircuit logic:

if (doSomethingWith(dao.getByID(12345))
 || doSomethingWith(dao.getByName("Joe Bloggs"))) {

Answer 3

First, I do echo the points raised by @rolfl and @maaartinus, and I also feel that there are probably other parts of your codebase that you need to review given the usage highlighted in your question. With that said, given the limited context that you have presented, I can think of a third alternative.

Can you create 'lite' instances of your Product class that only contain 'one of these properties' for your existing class to use?

For example, if one part of your client code only knows the web ID:

Product lookupByWebId = Product.ofWebId("WebID");

That will create a Product object where only the webId field is populated and the other fields null. Your existing class can then have this method:

public final boolean isExisting(Product product) {
    // ignoring getters for illustration purposes
    if (product.id != null) {
        return dataAccessObject.findById(product.id) != null;
    }
    if (product.name != null) {
        return dataAccessObject.findByName(product.name) != null;
    } 
    if (product.displayName != null) {
        return dataAccessObject.findByDisplayName(product.displayName) != null;
    }
    if (product.webId != null) {
        return dataAccessObject.findByWebId(product.webId) != null;
    }
    throw new IllegalArgumentException("No queryable fields.");
}

The benefit here is that when you enhance your Product class with newer fields, you do not have to change method signatures to accommodate them.

If you do however find a way to even workaround the need to keep this kind of 'check' methods as suggested in @rolfl and @maaartinus answers, then please do that instead. :)