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?

Improve this question
4
  • 2
    example as written suggests you could simplify it to just showSomeMessageToUser(entity.name) I'm assuming the message is different? Commented May 15, 2018 at 17:37
  • there was a mistake Commented May 15, 2018 at 17:40
  • 3
    What do you believe is the problem with this code? Commented May 15, 2018 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. Commented May 15, 2018 at 18:14

3 Answers 3

Reset to default
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.

Improve this answer
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.

Improve this answer
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.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.