4

There's a common code smell involving long methods with the most common answer being that methods should be really small, less than 50 lines per say (or 20). I understand why this is because it enhances readability, reduces repeated code, etc. However, I was wonder if this is at a statement level or at a file line level.

For example, let say I had a method that was being called and it had a lot of parameters passed (ignore the LPL smell for this example), too much that it would span across the width of a common editing window size in an IDE.

method(A, B, C, D, ...);

EDIT Envision the lettered parameters to be some typed variable, ellipses indicates further parameters.

However, somebody may write the method call like this

method(
A,
B,
C,
...);

Now this spans multiple lines, but in my personal opinion, its far easier to read scrolling down than across. The actions on the mouse are also different since scrolling side to side often requires a mouse select of the (left to right) scroll bar (I'm aware a button could be programmed to do this and some mouses have a joystick built in, but it isn't common in business).

I find this to be relevant for SQL queries. For example

SELECT 
 A.A
,A.B
,B.C
,B.D
FROM 
table A
LEFT JOIN
table B
ON
A.E = B.E
AND 
A.F = B.F
WHERE 
A.A IS NOT NULL
AND 
B.C > 50

Now in this scenario, when I'm testing, its very easy to comment out where clauses or change logic operators by commenting them out instead of performing deletes if it was all on one line and had to re-type.

I guess I was wondering what long line method smell really means and whether its long lines in terms of many statements or long lines in terms of lines per file?

candied_orange
  • 102,279
  • 24
  • 197
  • 315
Jake
  • 151
  • 3
  • Possible duplicate of [Should a method do one thing and be good at it?](https://softwareengineering.stackexchange.com/questions/137941/should-a-method-do-one-thing-and-be-good-at-it) – gnat Mar 22 '18 at 16:21
  • 1
    Possibly relevant: [How would you know if you've written readable and easily maintainable code?](https://softwareengineering.stackexchange.com/q/141005/64132) – Dan Pichelman Mar 22 '18 at 16:22
  • I appreciate the links, but they don't really answer my question specifically. For example, a code review is subjective and someone could easily say writing a query on a single line is easier to read. Then again, I guess I'm looking for an opinion anyways... Also not sure how this is a duplicate of [Should a method do one this and be good at it?](https://softwareengineering.stackexchange.com/questions/137941/should-a-method-do-one-thing-and-be-good-at-it) considering use of white space has nothing to do with that. – Jake Mar 22 '18 at 17:21
  • That you have a quandary about this should make you question the wisdom of taking such a literal interpretation of someone's definition of what's smelly, especially one with an arbitrary number like 20 or 50. Related, but probably not a duplicate: https://softwareengineering.stackexchange.com/questions/231427 – Blrfl Mar 22 '18 at 18:05
  • Too much whitespace reduces the amount of code you can see, hurting readability. – Reinstate Monica Mar 22 '18 at 19:11
  • IMO the goal is to have your entire method be viewable in an editor without having to scroll. That's how you get to 20-50 lines. On a side note, I find it annoying when people put a blank line between every line of code. Use blank lines to separate code into logical groupings instead. – JimmyJames Apr 15 '19 at 17:29

2 Answers2

12

Is using spacing effectively equivalent to the long method code smell?

No.

Never let line number concerns eliminate whitespace. If anything, this shows that line counting is not a good quality metric.

Methods and functions should be short. Very short. If you can break them down into anything smaller it's worth considering doing just that.

But line counting is really silly. Especially in languages that ignore whitespace. I can define an entire program in one line. That doesn't make it good.

Now personally I find this example very readable:

method(A, B, C, D);

I don't find this very readable:

public String method(final String Alessandro , final String Bartholomew, final String Cleveland, final String Dashiell) { return ""; }

Even though they have the same number of parameters, this one is terrible. It would be worth adding whitespace to make it readable.

public String method(
        final String Alessandro, 
        final String Bartholomew, 
        final String Cleveland, 
        final String Dashiell
) {
    return "";
}

Never shorten a name because of width or length considerations. Use the best name you can and find a better way to lay out your code.

However, I find

method(
A,
B,
C,
...);

to be pointlessly wasteful of space. At the very least use some indentation.

method(
    A,
    B,
    C
);

With such short names and no other adornments this still seems silly. But at least it emphasises the structure. If you had a good reason to do this I wouldn't deny you the white space, but it's annoying if I can't see the reason.

SELECT 
 A.A
,A.B
,B.C
,B.D
FROM 
table A
LEFT JOIN
table B
ON
A.E = B.E
AND 
A.F = B.F
WHERE 
A.A IS NOT NULL
AND 
B.C > 50

This is just plain hard to look at. All you've done is allow for your commenting scheme. That doesn't make it read much better than

SELECT A.A, A.B, B.C, B.D FROM table A LEFT JOIN table B ON A.E = B.E AND A.F = B.F WHERE A.A IS NOT NULL AND B.C > 50

You still have the flexibility for other considerations. You can add some visual separation between keyword and parameters

SELECT 
     A.A
    ,A.B
    ,B.C
    ,B.D

FROM 
    table A

LEFT JOIN 
    table B

ON 
    A.E = B.E

AND 
    A.F = B.F

WHERE 
    A.A IS NOT NULL

AND 
    B.C > 50

You can emphasize the relationship between the keywords

SELECT 
,A.A 
,A.B 
,B.C 
,B.D

    FROM 
    table A

    LEFT JOIN 
    table B 

        ON 
        A.E = B.E

        AND 
        A.F = B.F

    WHERE 
    A.A IS NOT NULL

        AND 
        B.C > 50

And it may be worth remembering that comments don't have to take up an entire line:

SELECT A.A, /*A.B,*/ B.C, B.D
    FROM table A
    LEFT JOIN table B 
        ON A.E = B.E
        AND A.F = B.F
    WHERE A.A IS NOT NULL
        AND B.C > 50

Whitespace is powerful. It has a serious impact on readability. Use it wisely.

Counting lines is the worst lesson to take from learning that functions should be short. They shouldn't be short because you squished all the whitespace and descriptive names out of them. They should be short because you gave everything it's own little well named function to live in.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • Yeah, I was just writing pseudo code where A was supposed to be some typed variable. That wasn't clear in the question. Again, the indenting would probably help. Most of my sql procedures perform one single function (ignoring the error handling). – Jake Mar 22 '18 at 18:32
  • I think the SQL would be better if the blank lines inside the FROM and WHERE clauses was removed. – Sebastian Redl Mar 22 '18 at 20:12
  • Yeah, that's true. Didn't think about multi-line comments. Although I just find typing -- much easier than typing /* */ and I find it too easy to lose track of the closing */ causing much of your code to be commented unintentionally. But since everyone seems to like scrunched code, it wins out. – Jake Mar 22 '18 at 21:48
  • @Jake I don't mind fluffy code if it has a point. I can respect wanting to make commenting easy. While I detest line counting, I try to keep any one idea on one screen with no scrolling. Better to refactor than to scrunch names or whitespace. – candied_orange Mar 22 '18 at 22:55
  • I agree. Its just you can't really refactor something like a merge query which is designed to be a large statement that performs insert/updates/deletes at the same time. Especially when its using CTE's as its source/target for merging. You can either have a sprawling single line of columns or a multi-line of columns. The style provided by @Robert Harvey seems to be a good balance I think. – Jake Mar 23 '18 at 13:31
  • `If you can break them down into anything smaller it's worth considering doing just that` While I agree with the advice, I do want to sidenote here that this should not be taken to extremes either. Only do so when breaking things down further _simplifies_ things rather than adding complexity. Otherwise, you'll end up abstracting for abstraction's sake. Just because you can doesn't mean you should. – Flater Apr 15 '19 at 17:43
5

Your SQL example is terrible. It breaks things into individual lines unnecessarily, hurting readability without providing significant benefit.

A better arrangement is to line break on major keywords and provide sensible indentation, like this:

SELECT A.A, A.B, B.C, B.D
  FROM table A
  LEFT JOIN table B 
    ON A.E = B.E
    AND A.F = B.F
  WHERE A.A IS NOT NULL
    AND B.C > 50

Which doesn't hurt your commenting out and testing ability at all.

There are better ways of making long method calls more readable. For example:

var a = [long expression];
var b = [another long expression];
var c = ...

method(a, b, c, d, ...);
Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
  • It does when for example I want to comment out an AND and switch it to an OR in a join while keeping the join condition the same. It also makes it hard if I wanted to remove a column and quickly add it back in without having to retype if I wanted to see something during debugging. However, your indenting has given me some pause for thought. – Jake Mar 22 '18 at 18:28
  • 1
    You just need to be more creative in SSMS, that's all. For example, you can remove the first condition in the left join by higlighting `A.E = B.E AND`, hitting Ctrl-X, typing a `--` and Ctrl-V. Just takes a second or two. Good code formatting is not just about you; it's also about the other team members who have to read your code. – Robert Harvey Mar 22 '18 at 20:20
  • The second example makes it a lot easier to debug. If something is wrong, it could be a, b, c, d or method that’s wrong. Your example easily lets you check whether a to d are correct, or let’s you step directly into method, skipping the parameter evaluation. – gnasher729 Nov 17 '19 at 12:12