0

I encountered code like this below. I was told its fine, and boilerplate is not always better. I agree boilerplate might be bad, but I am not sure about something like this:

    if (entity.isSomething && wasCheckedBefore(entity.name)) showSomeMessageToUser(entity.name) 
    if (entity.isSomethinElse && wasCheckedBefore(entity.name)) showSomeMessageToUser(entity.name) 
    if (entity.isSomethingSomethinElse && wasCheckedBefore(entity.name)) showSomeMessageToUser(entity.name) 

What would you recommend? (rather not CoR or Command pattern)

//Edit I understand what do you mean by saying this code it's fine. What if there were more conditions etc? Whats the best way to go?

  • 2
    example as written suggests you could simplify it to just showSomeMessageToUser(entity.name) I'm assuming the message is different? – Berin Loritsch May 15 '18 at 17:37
  • there was a mistake – nodedotjs May 15 '18 at 17:40
  • 3
    What do you believe is the problem with this code? – Robert Harvey May 15 '18 at 17:43
  • 1
    If you rule out command pattern, about the only thing you can safely do is wrap the whole block with an if(wasCheckedBefore(enty.name)) and still have the individual if(enitity.isSomething) showSomeMessageToUser(entity.name) inside. That may be subjectively better in terms of readability, but it also might not. If you are going to make a fundamental change at least do something to make the intent more clear. Another option is to have an entity.Type be an enum so then you can do a switch statement. Again, whether it's better or not is subjective. – Berin Loritsch May 15 '18 at 18:14
5

I'll be honest, I don't see a whole lot wrong with the code sample you listed--assuming it's just a handful of cases like that. There are a few ways you can change the code that may or may not make the resulting code more readable or the intent more clear.

One example is to separate the common clause in the if statement from the rest of the code like this:

if (wasCheckedBefore(entity.name)) {
    if (entity.isSomething) showSomeMessageToUser(entity.name) 
    if (entity.isSomethingElse) showSomeMessageToUser(entity.name) 
    if (entity.isSomethingSomethinElse) showSomeMessageToUser(entity.name) 
}

Another option is to have an enum type parameter to do a switch statement:

if (not wasCheckedBefore(entity.name)) return

switch(entity.type) {
    case type.something:
        showSomeMessageToUser(entity.name)
    case type.somethingElse:
        showSomeMessageToUser(entity.name)
    case type.somethingSomethingElse:
        showSomeMessageToUser(entity.name)
}

Or extending that concept you can simply pass the type into the method so it really becomes shorter and the complexity moved to the showSomeMessageToUser function:

if (wasCheckedBefore(entity.name)) {
    showSomeMessageToUser(entity.type, entity.name)
}

The bottom line is that none of this is functionally any better than the other. It's just shuffling the logic around. The question is whether any of these approaches make the intent of the code more prevalent. If not, let sleeping dogs lie.

1

If there's a good domain term for what's being checked, you can convert each boolean expression to a method:

// This method name should explain what the boolean expression 
// means in terms of domain logic, i.e. why it is important
// to check both of these values.
private boolean someDomainConcept(entity) {
   return entity.isSomething && wasCheckedBefore(entity.name)
}

if (someDomainConcept(entity.name)) showSomeMessageToUser(entity.name);

This can add some documentation value to the code while making it a little more brief to use your boolean expressions and enabling reuse of those expressions. I wouldn't say that this is always a valuable pattern; it depends on your actual use case. Sometimes there isn't a better name for the boolean expression than just the sum of its parts.

0

Why don't you just consolidate your messages inside the entities?

class SomethingEntity
{
    string name;
    bool isChecked;

    public string Message()
    {
        if (isChecked)
            return someMessage;
        else
            return someOtherMessage;
    }
}

You can then

showMessageToUser(anyEntity.Message());

There are many ways to skin this cat. You probably don't need a well-known software pattern.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.