3

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?

10

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 function to live in.

  • 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
6

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, ...);
  • 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

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.