0

I have inherited a class method from another developer, which looks like below: (Note: Class:X means X is a member of Class in the OOP paradigm.)

Class:BigFunction() 
{
    ImportantValue = calculateImportantValue()

    Iterate over Class:Foo from 1 to ImportantValue
    Iterate over Class:Bar from 1 to ImportantValue
    Iterate over Class:Baz from 1 to ImportantValue

    # ... and so on ...
}

I am considering refactoring this to something like:

Class:WrapSeveralSmallFunctions() 
{
    ImportantValue = calculateImportantValue()

    IterateFoo(ImportantValue)
    IterateBar(ImportantValue)
    IterateBaz(ImportantValue)

    # ... and so on ...
}

Class:IterateFoo(ImportantValue)
{
    Iterate over Foo from 1 to ImportantValue
}

Class:IterateBar(ImportantValue)
{
    Iterate over Bar from 1 to ImportantValue
}

Class:IterateBaz(ImportantValue)
{
    Iterate over Baz from 1 to ImportantValue
}

Per Single Responsibility Principle, the refactored code looks better to me.

However, I am also concerned about passing the ImportantValue as an argument to every such "small" function. Is my concern valid? Is the proposed refactoring considered a good coding practice, or is there any better way of writing this code?

  • consider extract class refactoring – gnat May 28 '15 at 4:42
  • surely you should do only one loop 0 to importantValue and do all three things in it – Ewan May 28 '15 at 5:18
2

In term of performance, there will be virtually no difference between the two considered options.

Theoretically, the Single Responsibility Principle suggests that the solution with several small functions called by the big one is preferable if the small functions make sense. Are there semantically well defined? When you read the function names and the for-loops in the execution flow, do you read a consistent story?

If, taken individually, the small functions don't have a clear, well defined goal, your refactoring is probably too fine grained and counterproductive. Depending on your programming language, you can then consider to use advanced list comprehension or functional style coding for reducing verbosity.

For instance, the following code limits the handling of each collection to a single line, which is as simple to understand as a call to a small function with a for-loop.

Class:BigFunction()
{
    ImportantValue = calculateImportantValue()

    Class:Foo.take(ImportantValue).foreach(f => apply(f))
    Class:Bar.take(ImportantValue).foreach(f => apply(f))
    Class:Baz.take(ImportantValue).foreach(f => apply(f))
}

If you systematically have to process your three lists in multiple "big" functions, it can be an evidence that the class itself violates the SRP. You therefore should consider to split the class. This operation is also a refactoring.

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.