C# refactoring considerations

Question

I have the following doubt.

For refactoring, I have read that is good create methods that has a very specific responsability, so if it is possible, it is a good idea to split a complex method in others small methods.

But imagine that I have this case:

I have to create a list of objects, and insdie this objects, I have to create another object. Something like that:

public void myComplexMethod(List<MyTypeA> paramObjectsA)
{
    foreach(MyTypeA iteratorA in paramObjectsA)
    {
        //Create myObjectB of type B

        //Create myObjectC of type C

        myObjectB.MyPorpertyTpyeC = myObjectC;
    }
}

I can split this method in two methods.

public void myMethodCreateB(List<MyTypeA> paramObjectsA)
{
    foreach(MyTypeA iteratorA in paramObjectsA)
    {
        //Create myObjectB of type B
    }
}



public void myMethodCreateB(List<MyTypeB> paramObjectsB)
{
    foreach(MyTypeB iteratorB in paramObjectsB)
    {
        //Create myObjectC of type C
        iteratorB.PropertyC = myObjectC;
    }
}

In the second option, when I use two methods instead one, the unit tests are less complex, but the problem is that I use two foreach loops, so it is less efficient than use only one loop like in the first option.

So, what is the best practice, at least in general, to use a method a bit more complex to be more efficient or to use more methods?

Thanks so much.

It is unclear the relationship between myComplexMethod and myMethodCreateB overload 1 & 2 (i.e., both alternatives are not delivering the same). Please, explain better your exact situation (= write a code which shows how all these methods are actually connected). — user2480047
– user2480047, Commented Sep 5, 2015 at 11:55

Christoph · Accepted Answer · 2015-09-05 12:12:25Z

3

I generally put readability at a higher priority than performance, until proven otherwise. I'm generalizing now a bit but in my experience, when people focus on performance too much at the code level, the result is less maintainable code, it distracts them from creating functionally correct code, it takes longer (=more money), and possibly results in even less performant code.

So don't worry about it and use the more readable approach. If your app is really too slow in the end, run it through a profiler and pinpoint (and prove) the one or two places where it requires optimization. I can guarantee you it won't be this code.

Making the correct choices at the architectural level early on is much more critical because you won't be able to easily make changes at that level once your app is built.

answered Sep 5, 2015 at 12:12

Christoph

2,3671 gold badge19 silver badges28 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

user2480047 Over a year ago

How can you know wether the second alternative has a better or worse performance? These codes are not related at all. Presumably, the first alternative has two nested loops which are converted into two different functions in the second one, but this is just a guess. And the number of loops seems to remain constant anyway (otherwise how is the second version expected to deliver the same result?).

Christoph Over a year ago

The names are slightly different and could be improved by in my opinion it is clear enough. First example has one loop that creates both objects and performs parent-child assignment in that loop. Second example has two loops: Parent is created in first, child created and parent-child assigned in second.

user2480047 Over a year ago

Sorry Christoph but this is not possible: if the first code can create everything with just one loop (i.e., paramObjectsB is not a collection), the two loops in the second alternative wouldn't make sense. The only logical explanation is that there are two collections (paramObjectsA & paramObjectsB). In the first alternative there are two nested loops (not shown; it should be the //Create myObjectB of type B part) and in the second one the same via call to a function with two overloads. In any case, the OP should post an actually testable code (very far away from being the case here).

Christoph Over a year ago

Hmm, you're right. The given example doesn't make sense. In any case, I stand by my generalized opinion that it is better to choose maintainability and readability over premature optimization.

Enigmativity Over a year ago

@Christoph - Even though the OP's code doesn't quite make sense I think your answer is quite clear and applicable.

Ryan · Accepted Answer · 2015-09-05 12:57:28Z

2

Usually I would keep using one for-loop in this case. Seems you are just create and decorate the objects of MyTypeB. I would prefer create a factory method in class MyTypeB:

static MyTypeB Create(MyTypeA a) { // if the creation of MyTypeB depends on A
    //Create myObjectB of type B
    //Create myObjectC of type C
    myObjectB.MyPorpertyTpyeC = myObjectC;
    return myObjectB;
}

then your complex method will become:

public void myComplexMethod(List<MyTypeA> paramObjectsA)
{
    foreach(MyTypeA iteratorA in paramObjectsA)
    {
        MyTypeB myObjectB = MyTypeB.Create(iteratorA);
    }
}

answered Sep 5, 2015 at 12:57

Ryan

8158 silver badges14 bronze badges

Collectives™ on Stack Overflow

C# refactoring considerations

2 Answers 2

5 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

5 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related