HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
existencecheckingdisplaynameforwebobject

Problem

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 byId(Long id) {
return Optional.ofNullable(dataAcce

Solution

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 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 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.

Code Snippets

public final boolean doesExistById(Long id) {
    return dataAccessObject.findById(id) != null;
}
if (doesExistById(id)) {
    return getById(id);
} else {
    return new Something(id);
}
dataAccessObject.findById(id)
somethingContainingTheDataAccessObject.findById(id)
public final boolean doesExist(Long id, String name, String displayName, String webId) {..}

Context

StackExchange Code Review Q#96771, answer score: 6

Revisions (0)

No revisions yet.