1

Consider this bit of code :

private Norf foo(Baz baz) {
    // ...
    // Logic on baz
    // ...

    if (baz.color == Baz.BLUE) {
        // Do this thing
    }

    // ...
    // More logic
    // ...

    return norf;
}

Assume that the content of the if statement is at the wrong level of abstraction for foo and therefore ripe for refactoring. I often walk into situations like this, but I never feel like my refactoring is good.

One way to refactor would be to extract a doThisThingIfBazIsBlue function. A function with an "if" in its name smells bad to me though, so that's not good.

Another approach is to keep the conditional in foo and extract doThisThing. This solution feels better, but sometimes it's not really foo's business whether the "thing" is done or not. In this case foo is too short for this to matter, but in longer functions it can end up being the bulk of the complexity.

Do you know a naming pattern that works for this kind of situation? How do you refactor complex if statements and bodies?

Improve this question

4 Answers 4

Reset to default
6

One of the solutions to your problem could be polymorphism and delegating the // Do this thing, to the Baz class.

Then changing the implementation of the foo method to something like this:

private Norf foo(Baz baz)
{
    // ...
    // Logic on baz
    // ...

    baz.doThisThing();

    // ...
    // More logic
    // ...

    return norf;
}

And having multiple implementations of the Baz class.

class BlueBaz extends Baz
{
    public void doThisThing()
    {
        Console.WriteLine("Yay, this is right. Blue is the color of men!");
    }
}

class AnyOtherBaz extends Baz
{
    public void doThisThing()
    {
        throw new Exception("Sorry, no blue color in this one. Perhaps you like a different one.");
    }
}

And if you feel like you would still need an if check in the new children of the Baz class, in the new doThisThing method, you can delegate the decision even further, perhaps to a Color class.

The pattern you are seing here is called inversion of control. It basically means there is a predefined contract by a public API, mostly determined by interfaces, but sometimes determined by base (abstract) classes. The contract is set and can be used, but the implementations are chosen during runtime, when you are constructing the object graph.

About the naming convention

If you delegate the condition further down the road (or classes respectively), you will not really need to have an ifACondition in the name of your method, because the method itself will not contain the if you are worried about. To handle the if, you will have a completely separate class, having the doThisThing method but providing different implementation, different logic.

In your own case you would be left with the name doThisThing and decide which class would be chosen by constructing the desired instance (perhaps by a factory or in a boostrap being responsible for the construction of the object graph).

Aditional note

Take into consideration this is a possible approach, but not the only one. If you do not want to have conditionals in your code, polymorphism is the way to go in OO programming. But if you are fine with the alternative router when the color is blue and are satisfied with how the code works so far, you could leave it as it is.

Improve this answer
2
  • 1
    Using polymorphism is a good suggestion but throwing the exception makes it rather useless. (Now the caller has to use try and catch instead of an if which is even more verbose and probably less efficient.) Instead, AnyOtherBaz::doThisThing should simply do nothing. If the caller might be interested whether the thing was done, the functions could return a boolean.
    – 5gon12eder
    Feb 3, 2016 at 20:48
  • @5gon12eder It was merely a demonstration, that the function can do pretty much anything.
    – Andy
    Feb 3, 2016 at 20:54
2

I have this quite often as well.

doThisThingIfBazIsBlue smells funny. doThisThing is OK, but as you say, this logic should sometimes be encapsulated into the method.

I usually try to look at the operation from higher level view and see what it is intending to do and name it accordingly (I struggle to produce example now...).

Sometimes I just name it doThisThing and consider "if" as an internal implementation detail - action is done only if its necessary which simply doesn't need to be known to the client code (e.g. data from the DB is loaded only if it's not in the cache - client doesn't care about such implementation detail).

If above fails, I usually call it handleSomething, e.g. handleSaving - it's my internal coding convention - it's not as imperative as "save()" and may imply that there's some more logic/preprocessing to it.

Improve this answer
1
  • 1
    ensureCapacity instead of growIfNeeded might be an example you were thinking of.
    – 5gon12eder
    Feb 3, 2016 at 20:50
1

Probably a non-answer, but I wouldn't refactor this. Unless it shorten the code or makes it more readable, it just makes it more confusing.

Best lesson learned about refactoring is wait until you use something twice before making it a function. If you can use "doThisThingIfBazIsBlue()" function twice then separate it out and if not don't.

Also, don't have a class morph into something else based on it attributes. If Baz.color=BLUE completely changes behavior then think of creating a BlueBaz class. And then you move the "Logic on baz" and "More logic" into subroutines.

Improve this answer
2
  • Still a valid point. It's tempting to refactor endlessly. But how do you deal with long functions, if you usually only refactor things you use twice?
    – Mobius
    Feb 2, 2016 at 18:24
  • @Mobius, IMO, long functions are fine if they are doing one concise thing and written clearly. convertLeadToGold() can be long just comment each step. On the other hand doABunchOfThingsToBaz() is just never going to look good.
    – swdev
    Feb 2, 2016 at 18:34
0

A good function should perform a specific task in a higher level abstraction, it must also obey the rules of high cohesion and low coupling. The name of your function must be the task is performing. If you can not find a good name (a name with "if" is not a good name), then the block of code you are trying to extract is not a good candidate for a good function. Maybe you should include more code, or remove code, or even maybe leave as it is.

For example the following block of code :

...

if (file.Exists) {
//code for removing the file and making any other related changes
...
}

...

Can be extracted as function :

 removeFile(file);
Improve this answer

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