Object matching using generic method? [closed]

I'm going to call you out on this line:

return null;

I think this is the cause of all your problems.

If we get rid of the null checks later in your code everything collapses to a single boolean expression. If we avoid getter functions and use properties it becomes even more readable:

private boolean isProfileApplicableOnRequest(Profile profile, Request request) {

    return (false 
        || profile.Applicability.contains(BigInteger.ONE)
        || profile.ResourceTypes.contains(request.ResourceType)
        || profile.Operations.contains(request.Operation)
        || profile.ResourceIDs.contains(request.To)
        || profile.ReleaseVersions.contains(request.ReleaseVersionIndicator)
    );    
}

If you avoid setting things to null in the first place you wont need to constantly check for null. Your constructors can set empty lists for all those properties. The fact that you are returning null in your top level function, suggests that you are also doing it in whatever generates these objects.

Depending on language you might also be able to use null coalescence ?? or optional chaining ? to condense your code.

There are a bunch of problems with the code causing it to look really messy, because methods on the Profile class that return lists are allowed to return null. This, combined with null-checks on the Request class make the comparison logic almost incomprehensible. It is hard to see how this code can be cleaned up by only changing a single class.

Instead, reduce the null-checks. Ewan's answer mentions this, but doesn't give much guidance. Based on the names of classes, the Profile class seems like a domain type, or business class. While null values make sense for a Data Transfer Object, they don't make sense in a domain class for collection-like attributes. Fixing this will likely involve modifying the Profile class, and pushing the comparison logic into the Request class — even if the Request class is a DTO.

1. Eliminate null return values for lists in the Profile class. Return an empty collection (e.g., a List<T> with zero items in it). If you really cannot return a list with zero or more items in it, consider throwing an exception instead.

2. Move the comparison logic into the Request class. Remember, a Data Transfer Object is a general description of an object that gets serialized. It does not mean the object cannot have methods. It shouldn't if possible, but you are not breaking any laws by adding a convenience method to a DTO.

These two changes allow you to declutter the code.

public class Request {
    // properties
    // getters and setters

    public boolean appliesTo(Profile profile) {
        return listContains(profile.getApplicability(), BigInteger.ONE)
            || listContains(profile.getResourceTypes(), getResourceType())
            || listContains(profile.getOperations(), getOperation())
            || listContains(profile.getResourceIDs(), getTo())
            || listContains(profile.getReleaseVersions(), getReleaseVersionIndicator());
    }

    private boolean listContains(List<BigInteger> list, BigInteger value) {
        return value != null && list.contains(value);
    }

    private boolean listContains(List<String> list, String value) {
        return value != null && list.contains(value);
    }
}

And in the class that processes the request:

public Response apply(Request request) {
    ..// some logic
    Profile profile = getProfile();

    if (request.appliesTo(profile))

    }

    return null;
}

Now you can get rid of all those pesky procedural methods that do not rely on internal state:

• isProfileApplicableOnRequest
• profileMatchesResourceType
• profileMatchesOperation
• profileMatchesToParameter
• profileMatchesReleaseVersion

That will really clean up the class that processes requests.

Bonus points for writing unit tests for Request.appliesTo(Profile) if both Profile and Request had simple dependencies, or dependencies that could be mocked.

If you haven't succumbed to primitive obsession and those request features each have their own type you could take advantage of argument type method overloading and do this:

List<BigInteger> applicability =  profile.getApplicability();
return 
    ( applicability != null && applicability.contains(BigInteger.ONE) )
    || profile.has( request.getResourceType() )
    || profile.has( request.getOperation() )
    || profile.has( request.getTo() )
    || profile.has( request.getReleaseVersionIndicator() )
;

The type overloading will let you look for the passed argument in the appropriate collection. Could have added profile.has(BigInteger.ONE) except that counts as a primitive, not a domain object. Without wrapping BigInteger.ONE in some domain type there's no reliable way for profile.has() to realize that it should look for a match in getApplicability().

Alternatively you could recognize that there's one more use for your null safe generic method. If you're willing to live with it having a different name:

return 
      contains( profile.getApplicability(), BigInteger.ONE )
   || contains( profile.getResourceTypes(), request.getResourceType() )
   || contains( profile.getOperations(), request.getOperation() )
   || contains( profile.getResourceIDs(), request.getTo() )
   || contains( profile.getReleaseVersions(), request.getReleaseVersionIndicator() )
;

Ewan is right to question the need to leave your collections null. However, I don't think the suspect code is return null. More likely, if you really are leaving collections null, and not just null checking out of paranoia, the offending code most likely looks like this:

List<Application> applicability;

Instead, make it look like this:

List<Application> applicability = new ArrayList<>();

Set up your collections as initialized member variables like that in Profile and they will only be left null when an out of memory exception is on the way up. You shouldn't need to check if they're null. They'll exist when a Profile instance exists.

I'll admit I'm slightly surprised that all you care about is that at least one matching thing is found (||) rather than that they all match (&&). But if that's really what you want, would you be interested in knowing not only that there was a match but exactly what matched?

The old school trick is to set a different bit for each one in a bitfield. If after all the checks the bitfield is 0 then nothing matched. If it's nonzero it's value tells you what matched. Make sure you have an enum for zero.

return
      profile.has( new Application(BigInteger.ONE) )
    | profile.has( request.getResourceType() )
    | profile.has( request.getOperation() )
    | profile.has( request.getTo() )
    | profile.has( request.getReleaseVersionIndicator() )
;        

There is also a new fangled way to do this using a little gem called EnumSet.

return EnumSet.of(
     profile.has( new Application(BigInteger.ONE) ),
     profile.has( request.getResourceType() ),
     profile.has( request.getOperation() ),
     profile.has( request.getTo() ),
     profile.has( request.getReleaseVersionIndicator() )
);

Looks pretty and despite that, is supposed to be efficient.

Excuse me while I steal one of @GregBurghardt's ideas:

Encapsulation can simplify this code:

public class Profile {
    private boolean isApplicable(Request request) {
        return (false 
            || applicability.contains(BigInteger.ONE)
            || resourceTypes.contains(request.ResourceType)
            || operations.contains(request.Operation)
            || resourceIDs.contains(request.To)
            || releaseVersions.contains(request.ReleaseVersionIndicator)
        ); 
    }
}

By making this the responsibility of Profile it no longer has to expose it's collections to the world. In fact, it's free to use something besides collections if it wants.

Regarding edit 6:

Currently in my implementation, we have a standard XSD/JSON Schema defined by the specifications and I generate Java classes from Schema and keeping those in a jar file.

So both Request and Profile are part of the generated Java classes. So for Request if we want more member variables we have created a wrapper around it extending the default generated Request object. So it's treated like a DTO currently. Profile is kept as it was generated which holds standard just getter/setters.

So currently, its mostly suggested in answers to shift logic to Profile class as it seems like a Domain object. But in CRUD request for Profile resource, it's passed in the payload over the wire.

So will it be recommended to create a wrapper for Profile and add domain logic methods to it (keeping transient variables if newly added variables) ??

A wrapper certainly lets you introduce methods. It will cause coupling between Profile and it's wrapper. But that keeps that same coupling from spreading to everything that would have used Profile directly that can now use the wrapper.

Wrapping DTOs in fully encapsulating objects is a classic pattern. Sure the DTO is breaking encapsulation but if you limit what touches it directly you limit the damage that causes.

DTO's are required when crossing a boundary that wont let you move methods across it. So don't be surprised if you eventually have to provide a way for the wrapper to send that DTO somewhere else. That's OK. That's not why you do this. You do this to simplify the code that now doesn't have to talk directly to the DTO.