Search code examples
functionrefactoringcoding-style

When is a function too long?

35 lines, 55 lines, 100 lines, 300 lines? When you should start to break it apart? I'm asking because I have a function with 60 lines (including comments) and was thinking about breaking it apart.

long_function(){ ... }

into:

small_function_1(){...}
small_function_2(){...}
small_function_3(){...}

The functions are not going to be used outside of the long_function, making smaller functions means more function calls, etc.

When would you break apart a function into smaller ones? Why?

  1. Methods should do only one logical thing (think about functionality)
  2. You should be able to explain the method in a single sentence
  3. It should fit into the height of your display
  4. Avoid unnecessary overhead (comments that point out the obvious...)
  5. Unit testing is easier for small logical functions
  6. Check if part of the function can be reused by other classes or methods
  7. Avoid excessive inter-class coupling
  8. Avoid deeply nested control structures

Thanks everyone for the answers, edit the list and vote for the correct answer I'll choose that one ;)

I am refactoring now with those ideas in mind :)

Solution

  • There aren't any real hard or fast rule for it. I generally like my methods to just "do one thing". So if it's grabbing data, then doing something with that data, then writing it to disk then I'd split out the grabbing and writing into separate methods so my "main" method just contains that "doing something".

    That "doing something" could still be quite a few lines though, so I'm not sure if the "number of lines" metric is the right one to use :)

    Edit: This is a single line of code I mailed around work last week (to prove a point.. it's definitely not something I make a habit of :)) - I certainly wouldn't want 50-60 of these bad boys in my method :D

    return level4 != null ? GetResources().Where(r => (r.Level2 == (int)level2) && (r.Level3 == (int)level3) && (r.Level4 == (int)level4)).ToList() : level3 != null ? GetResources().Where(r => (r.Level2 == (int)level2) && (r.Level3 == (int)level3)).ToList() : level2 != null ? GetResources().Where(r => (r.Level2 == (int)level2)).ToList() : GetAllResourceList();