11

I try to follow the Single Responsibility Principle (SRP) and also to omit Code Repetitions. However there are often places where there are code repetitions that are nothing more than code blocks of invocations that are resistant to extracting them into at least meaningfull named method:

DoAction1();
DoAction2();

if (value)
    DoAction3();

DoAction4();

What is the best way to extract such code into a method and how to name it?

migrated from codereview.stackexchange.com Nov 22 '13 at 18:04

This question came from our site for peer programmer code reviews.

  • 1
    Why are they resistant to being extracted into a different meaningful method? – Chandranshu Nov 22 '13 at 9:34
  • All actions do something unrelated. I would have to write: void MethodThatDoAction1ThenAction2AndAction3IfValueAndThenAction4(). I would rather see more meaning in: CodeBlock1(). – yBee Nov 22 '13 at 9:47
  • 4
    Then it's merely a question of properly naming the extracted method, right? I'm afraid there is no universal solution and you'll have to do this on a case by case basis. If you are repeating this exact same piece of code in more than 1 place, there is bound to be a meaning to your actions and you need to look hard (may be discuss) to find that meaning and give it a name. – Chandranshu Nov 22 '13 at 9:51
  • 1
    If it's any consolation, the Spring winner for long class names is AbstractInterruptibleBatchPreparedStatementSetter which is only a tad smaller than your method. – Chandranshu Nov 22 '13 at 9:56
  • 1
    @Chandranshu I believe your comment(s) would work fine as an answer to this question. – Simon Forsberg Nov 22 '13 at 12:46
13

I think it's merely a question of properly naming the extracted method. I'm afraid there is no universal solution and you'll have to do this on a case by case basis. If you are repeating this exact same piece of code in more than 1 place, there is bound to be a meaning to your actions and you need to look hard (may be discuss) to find that meaning and give it a name.

If it's any consolation, the Spring winner for long class names is AbstractInterruptibleBatchPreparedStatementSetter which stands at 49 characters.

11

Actually you are suffering from the top hardest task to do by a programmer - Naming Things. Sorry for such an amature answer but I couldn't resist. You can find that many people suffer from this, even in internet you can find this type of essay.

Now if your repetitive code is all together resulting one work and those methods are not being used by other methods then they are simply Helpers method and then the resulting method can be named as doXXX or makeXXX... I think you get the point.

As Chandranshu said "If you are repeating this [...] meaning and give it a name." is to the point and appropriate.

enter image description here

Pic courtesy : CodeChef Facebook Page Photo

2

I think you are taking the Code Repetition principle too far. Think of the point of avoiding Code Repetition. The point is to reduce the amount of code that has to be checked when there is a change in the logic, and to increase comprehension by factoring out obviously similarly-intended blocks.

The downfalls of factoring out in order to avoid Repetition are that if one of the shared blocks has to change, now you need even more complex inheritance or some switching between standard and non-standard implementation.

So carefully weigh the possibility of the logic for even one of these blocks changing without the others against the comprehension benefits gained by factoring this commonality out. If one implementation could split from the others you may very well be better off simply repeating the code.

While maintaining this repeated code, as it becomes more complex and your problem domain becomes more defined, you may then find it more appropriate to factor the repeated, now more complex, but also more defined sections out.

I usually try to maintain text-editor same-ness for a while until I can see whether something that looks to be repetitive turns out to be worth factoring. I just keep the repetition, but I keep my eye toward the future of that block by keeping it textually easy to match up later.

A lot of the time, the same-ness, and the possible factoring, starts to dissipate, as real, capricious, business rules and highly dependent, often arbitrary logic; like dealing with the oddities of several common database implementations (ANSI_NULLS or some such comes to mind) are added; forcing what seemed like a pure logic into a twisted mess, trying to provide reasonable, defensible decision logic when confronted with the mess of the state of the industry.

It seems to me that if people tried to factor out what you are trying to factor, we would have an entire library of worthless constructs like Do1Then2If2False Do1IfTrueDo2.

It has to more complex and more clear that the block is not going to change to warrant factoring it out.

It's software. You can go back and edit a couple blocks that are the same right now. It'll take 5 minutes. And you may save hours of wasted factoring, and then hours more of wasted inheritance and switching development, by just leaving it, and making sure you have a good , anti-RSI keyboard.

  • I like the part that extracts the essence of code repetition: to reduce the amount of code that has to be checked when there is a change in the logic – yBee Nov 26 '13 at 7:32
1

If you're truly following Single Responsibility Principle, then this block of code must have some specific purpose, right? Otherwise these methods would be in separate classes.

So then, what is the purpose of this code block? Determine that and you ought to be able to come up with a name.

If you still cannot figure out the name for this thing, then maybe these methods don't actually belong together at all. I.e. this class/code block has more than one responsibility. In that case, refactoring to split up the tasks may reveal better purpose-revealing name.

1

I've had a situation which may be similar to yours:

There's a class that defines functionality of the object that it represents:

class Functionality
{
protected:
void functionA();
void functionB();
...
void functionZ();
}

Then there's a class that defines the workflows for high-level operations that the object performs:

class Workflows: private Functionality
{
    void WorkflowA()
    {
        functionA();

        if (m_bValue) {
            functionB();
        }

        functionC();
    }
    ...
    void WorkflowB();
}

If you're in a similar situation, then identify what your classes represent (in this case functionality/workflows) and then name the methods accordingly.

Disclaimer: The class names used in this example are grossly inaccurate, but the method names give a clue. Discretion advised.

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.