Is having Constants in Interfaces for boolean parameters of Interface methods a good practice or code smell?

Question

So I have the following Interface:

public interface RoleService {
    boolean INCLUDE_DEACTIVE_OBJECTS = true;
    boolean EXCLUDE_DEACTIVE_OBJECTS = false;
    Set<? extends BaseBusinessObject> getOwnedBusinessObjectsOf(final Employee employee, final boolean includeDeactiveObjects);
}

and somewhere in an upper layer, an example usage is as follows..

if (someCondition) {
    ownedBusinessObjects = roleService.getOwnedBusinessObjectsOf(employee, RoleService.INCLUDE_DEACTIVE_OBJECTS);
} else {
    ownedBusinessObjects = roleService.getOwnedBusinessObjectsOf(employee, RoleService.EXCLUDE_DEACTIVE_OBJECTS);
}

So instead of passing values such as true (or false), I believe it is much easier to read the method call when I say INCLUDE_DEACTIVE_OBJECTS.

But I am not sure, is this just plain stupid? Is this an anti-pattern or a code smell or some sort of violation of a best practice?

I think this resembles avoiding Magic Numbers somehow, but is it as useful or is it rather confusing?

Kudos for `language-agnostic` tag. It is rarely seen these days on design questions. — Nikhil Vartak, May 30 '17 at 14:57

deceze · Accepted Answer · 2017-05-30T07:41:15.653

Since this makes the API more readable, it's fine. It doesn't matter what the underlying value is, it matters that you pass a flag that triggers a certain behaviour. If your language of choice only allows positional argument passing, then such a constant makes the call much more readable. Compare:

roleService.getOwnedBusinessObjectsOf(employee, RoleService.INCLUDE_DEACTIVE_OBJECTS);
roleService.getOwnedBusinessObjectsOf(employee, true);

"Get owned business objects of employee… what?" Nobody has any idea what "true" means here, a named value is much better.

The alternative is when your language of choice supports call time named arguments (here, Python):

roleService.getOwnedBusinessObjectsOf(employee, include_deactivated=True);

The API should enforce named arguments here (again: Python):

def getOwnedBusinessObjectsOf(employee, *, include_deactivated): ...

In Objective-C you'd do something similar with explicit method naming:

[roleService getOwnedBusinessObjectsOf:employee includingDeactivated:YES]

score 1 · Answer 2 · answered May 30 '17 at 15:28

Putting constant fields inside an Interface is definitely bad, IMO. I come from C# background and it does not even allow const fields in an Interface. Even in Java you have Constant Interface (anti-pattern again ) whose sole purpose is to contain constants that multiple classes can share. You're not exactly doing that stuff. Still just though of referencing.

So instead of passing values such as true (or false), I believe it is much easier to read the method call when I say INCLUDE_DEACTIVE_OBJECTS

Option 1

To replace constants whilst maintaining readability you could have define 2 methods which indicate if deactive (I would choose 'Inactive' instead. There's deactivate verb but no deactive adjective) objects are included.

getOwnedBusinessObjectsIncludingInactive(...)
getOwnedBusinessObjectsExcludingInactive(...)

These would be just wrapper methods in RoleService class that privately call a method to retrieve actual data, by passing true or false respectively.

Option 2

Have a separate class that defines application level constants in one place.

getOwnedBusinessObjectsOf(employee, Constants.INCLUDE_INACTIVE_OBJECTS)
getOwnedBusinessObjectsOf(employee, Constants.EXCLUDE_INACTIVE_OBJECTS)

score 0 · Answer 3 · answered May 31 '17 at 22:09

Any method which takes a boolean parameter violates the Single Responsibility Principle because it does one thing for true and a second thing for false.

This is always a code smell. Clean Code tells us to split the method in two:

getOwnedBusinessObjectsIncludingDeactiveObjectsOf(Employee employee);
getOwnedBusinessObjectsExcludingDeactiveObjectsOf(Employee employee);

Of course if the method is to be widely used, you will likely want more concise names, e.g. getAllObjects and getActiveObjects.

score 0 · Answer 4 · edited Dec 13 '19 at 13:56

If you don't want to use words with opposite meanings in your constant naming, you could also try wrapping booleans into an enum.

public interface RoleService {

    /**
    * Boolean wrapper for easy reading of values in method parameters.
    */
    enum IncludeDeactiveObjects {

        TRUE(true),
        FALSE(false);

        private final boolean flag;

        private IncludeDeactiveObjects(boolean flag) {
            this.flag = flag;
        }

        public boolean get() {
            return flag;
        }
    }

    Set<? extends BaseBusinessObject> getOwnedBusinessObjectsOf(final Employee employee, final IncludeDeactiveObjects includeDeactiveObjects);
}

The calls would be:

roleService.getOwnedBusinessObjectsOf(employee, RoleService.IncludeDeactiveObjects.TRUE);
roleService.getOwnedBusinessObjectsOf(employee, RoleService.IncludeDeactiveObjects.FALSE);

score -1 · Answer 5 · answered May 31 '17 at 11:34

If it is only for readability, in your abstract class, you can have static variable to hold these boolean values and you can assign with ClassName.AttributeName

public YourClass
{
    public bool static INCLUDE_DEACTIVE_OBJECTS = true;
    public bool static EXCLUDE_DEACTIVE_OBJECTS = false;

    public static void Main (String args[])
    {

    }
}

Is having Constants in Interfaces for boolean parameters of Interface methods a good practice or code smell?

5 Answers5