46

So, I watched as my colleague complained a bit about a project he has inherited from someone who is, shall we say, not very experienced as a programmer (intern left to his own devices on a project).

At one point there was duplicate code, about 7-8 lines of code duplicated (so 14-16 lines total) in a method of say 70-80 lines of code. We were discussing the code, and he spotted a way to refactor this to remove the duplication by simply altering the structure a bit.

I said great, and then we should also move the code out to a separate method so the large method gets a little more readable.

He said 'no, I would never create a method for just 7-8 lines of code'.

Performance issues aside, what are your input on this? Do you lean against using more methods (which in c# pads code with about 3 lines) or larger methods, when that particular code will probably not be used anywhere else? So it is purely a readability issue, not a code-reuse one.

Cheers :)

Max
  • 2,039

6 Answers6

84

The LoC in a method is a completely pointless measure. The important things are separation of concerns and code duplication.

A method should only do one thing, and that one thing should be expressed in its name. Other things should be left to other methods.

The problems arising from code duplication cannot be overestimated (in other words, they are always bigger than you think). And that alone will justify even methods with 1 line of code.

On the other hand, a method that populates a big form with details from a big data object can easily have 50 or more lines of code, but without a single conditional statement. Why would you want to break that down into smaller pieces if there is no functional requirement (or code duplication)?

wolfgangsz
  • 5,363
25

Shorter methods are very much more readable - fewer things to keep in mind to understand them. The most vocal advocates of clean code would probably say that almost all methods should be shorter than 8 lines. And pretty much every developer who's interested in the topic will agree that 70-80 lines of code in a method is too much and it should be broken up.

Michael Borgwardt
  • 51,502
8

It's all about separation of concerns. The method should answer 1 question or make 1 decision. And that's all. So, having the methods of 100 lines of code means these method are just do too much. 7-8 LoC methods are absolutely great. The other question is - where are you going to put all these methods. Having 20 short methods in a single class is probably a topic for concern. Having 30 of them is, I believe, the good reason to understand something's wrong with abstractions. Just keep everything as short and simple as possible.

Andrey Agibalov
  • 1,574
6

There's a school of thought that says 7-8 LOC is a bit long for a method. I don't subscribe to that but I'd say go right ahead - anything less than a screen is good for me.

mcottle
  • 6,152
  • 2
  • 26
  • 27
5

Methods should be as simple as necessary, but no simpler.

Andrew Lewis
  • 533
5

LoC is simply not a valuable metric. Instead, look to how much a method does. A method should generally do one thing. IMHO, it is sometimes ok to tack on 1 or 2 very simple operations. For example, calculating the perimeter of a rectangle:

It could be:

public int GetPerimeter(int side1, int side2)
{
    totalWidthLength = side1 * 2;
    totalHeightLength = side2 * 2;
    return totalWidthLength + totalHeightLength;
}

-OR-

public int GetPerimeter(int side1, int side2)
{
    return DoubleSide(side1) + DoubleSide(side2);
}

private int DoubleSide(int side)
{
    return side * 2;
}

Personally, I prefer the first method, even though it technically performs more than one operation. At the same time, you can also have methods with many lines of code that are simple to read and understand.

public void UpdateCustomer()
{
    customer Customer = new Customer();

    customer.Name = textboxName.Text;
    customer.AddrLine1= textboxName.Text;
    customer.AddrLine2 = textboxName.Text;
    customer.OfficialName = textboxName.Text;
    customer.Age = textboxName.Text;
    customer.BirthDay = textboxName.Text;
    customer.NickName = textboxName.Text;
    customer.LastLocation = textboxName.Text;
    customer.... = textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer.... = textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer.... = textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;
    customer....= textboxName.Text;

    db.InsertOnCommit(customer)
    db.SubmitChanges();
}

The above example is about 50 lines, but it could even be 200 lines, and would make no difference. Each property assignment on the customer object adds practically no complexity, so the 50 line method remains at the same complexity level as your average 4 line method. In fact, I would go so far as to say that it has basically the same readability as the method below:

public void UpdateCustomer()
{
    customer Customer = new Customer();

    customer.Name = textboxName.Text;

    db.InsertOnCommit(customer)
    db.SubmitChanges();
}

The 50 line method could probably be acheived in under 10 lines through the use of reflection and regex, iterating over each of the properties in Customer and searching for a textbox that has a matching LIKE value in its name. Obviously, that would be an example of code that is way too clever and resorts to wacky hacks to acheive an asthetic ideal. The bottom line is that LoC is an extremely unreliable complexity/readability metric. Everyone already knows this (my CRUD example is in no way uncommon), and yet while most people talk about how horrible LoC is at measuring productivity, we constantly hear about this LoC per method rules that are supposed to measure code quality. It really cannot be both ways.

GraniteRobert
  • 363