9
votes
\$\begingroup\$

Often I generate long lines of code such as the following...

shippedItems.AddRange(OrderItem.Fetch(market: this.MARKET, shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE, orderPlacedAfter: serverTime.AddDays(-7), orderPlacedBefore: serverTime.AddHours(-85)));

... which adds the results of a method call to an existing list.

Adding white space to this line could improve readability. At one point in time or another have rationalized almost every possible behavior between:

  • Leave everything on a single line and let the editor wrap where it feels is best.
  • Put even shippedItems.AddRange( on a line by itself.

While over time I feel that the clarity and readability of the code that I write has improved -- and lets hope for reasons other then white space -- I have never come to peace with how to break long lines.

I will up vote any answer that does NOT include as the solution:

  • shorter variable names.
  • disregarding named parameters (at least for this example).
  • creation of variables only used once.
\$\endgroup\$

locked by Jamal Mar 1 '15 at 6:53

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 2
    \$\begingroup\$ There's nothing wrong with creating a variable that is only used once. Sometimes, the required indentation is just too much. \$\endgroup\$ – EndangeredMassa Feb 14 '11 at 18:37
  • \$\begingroup\$ I am curious as to why you would not use a Pretty Print preprocessor to do this for you. \$\endgroup\$ – user1707 Feb 20 '11 at 21:14
  • \$\begingroup\$ What would StyleCop do? \$\endgroup\$ – Leonid Aug 17 '12 at 5:03
22
votes
\$\begingroup\$

I would break it up something like this:

shippedItems.AddRange(
    OrderItem.Fetch(market: this.MARKET,
                    shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
                    orderPlacedAfter: serverTime.AddDays(-7),
                    orderPlacedBefore: serverTime.AddHours(-85)));

Depending on previous indentation, some lines might flow over the "max line length", but I think that characters per line is more of a suggestion and there are good times to break that rule because breaking it leads to code that is more readable than code that doesn't.

Rules that I find helpful:

  • New line after an open paren.
  • Line breaks after commas.
  • Indent the inner method calls.
  • Line up parameters to a method that are on new lines.
  • Break "max line length" rules if it means the code is more readable.
\$\endgroup\$
  • \$\begingroup\$ Top of the pile after 1 week. Accepted on the basis of most preferred. \$\endgroup\$ – jphofmann Feb 21 '11 at 15:13
12
votes
\$\begingroup\$

As a follow up to @Thomas Owens, another rule I personally like to follow is:

  • Either put all parameters for a method on the same line, or put each parameter on its own line.

So I would write the code as follows. I like this because it makes reading the parameters more consistent, and doesn't indent them quite as far.

shippedItems.AddRange(
    OrderItem.Fetch(
        market: this.MARKET,
        shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
        orderPlacedAfter: serverTime.AddDays(-7),
        orderPlacedBefore: serverTime.AddHours(-85)));
\$\endgroup\$
  • \$\begingroup\$ I sometimes do this, myself. I suppose it depends on how many parameters I have and how long each one is. \$\endgroup\$ – Thomas Owens Feb 14 '11 at 16:18
  • \$\begingroup\$ I think this works well with named parameters, but without readability breaks down when a parameter is another function call. \$\endgroup\$ – jphofmann Feb 14 '11 at 16:40
  • 2
    \$\begingroup\$ +1 as I find it annoying to have to fix the leading whitespace after a rename/refactor to keep things lined up. I might put OrderItem.Fetch( on the line above for compactness, and I put the ending )); on its own line, outdented from the parameters (indented only one tab stop). \$\endgroup\$ – David Harkness Feb 15 '11 at 6:37
  • \$\begingroup\$ This is definitely my preferred way of writing that kind of code \$\endgroup\$ – Pete Feb 15 '11 at 19:42
8
votes
\$\begingroup\$

Interesting to see the range of responses. I would tend towards a different answer from any of those so far:

shippedItems.AddRange(OrderItem.Fetch(
    market: this.MARKET,
    shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
    orderPlacedAfter: serverTime.AddDays(-7),
    orderPlacedBefore: serverTime.AddHours(-85)
));

This to me reads more intuitively as "add the fetched order items to shippedItems, using the following block of arguments to fetch".

One piece of advice from someone who has been through the same dilemma for a while now: don't try to put rules on it. Take each example on its own merit and try to write it in the way that you would want it to be written if you were someone else trying to figure out what it does.

Sometimes it is a good idea to have a use-once variable to make something more readable. For example, if AddRange above had a second argument, where would you put it? Even this simple case reads badly

shippedItems.AddRange(
    OrderItem.Fetch(
        market: this.MARKET,
        shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
        orderPlacedAfter: serverTime.AddDays(-7),
        orderPlacedBefore: serverTime.AddHours(-85)
    ),
    2
);

However, this reads just fine

var orderItems = OrderItem.Fetch(
    market: this.MARKET,
    shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
    orderPlacedAfter: serverTime.AddDays(-7),
    orderPlacedBefore: serverTime.AddHours(-85)
);
shippedItems.AddRange(orderItems, 2);

Each case on its own merit.

\$\endgroup\$
  • \$\begingroup\$ +1, I use the One-True-Brace style (1TBS), so all the answers where the closing parenthesis and semicolon are indented make me cringe. Can't match the semicolon with the opening of the statement to tell where the multi-line statement ends. This looks much better. That being said, the 2nd example with 2 as a second parameter looks fine to me. \$\endgroup\$ – DCShannon Aug 26 '14 at 0:47
3
votes
\$\begingroup\$

This may be a sign you need to factor out the Fetch into a new method:

....
shippedItems.AddRange( LastWeeksMarketItems() );
....

private OrderItems LastWeeksMarketItems ()
{
    return OrderItem.Fetch(market: this.MARKET,   
                           shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,    
                           orderPlacedAfter: serverTime.AddDays(-7), 
                           orderPlacedBefore: serverTime.AddHours(-85));
}

The method name gives you further clarity about what it is you are trying to add.

\$\endgroup\$
  • \$\begingroup\$ For me, this just abstracts away what is actually being added to shippedItems. Also, this ends up defining in two places, to varying degrees, what items are being added. In the method name as "last weeks" and in the parameters being passed into fetch. \$\endgroup\$ – jphofmann Feb 16 '11 at 21:32
  • \$\begingroup\$ It depends on the usage to determine how useful it is. Think of it like a drill down. If you are adding last weeks in one place, last months in another and last years in another then you can quickly see which is being achieved. \$\endgroup\$ – Mongus Pong Feb 16 '11 at 21:56
  • \$\begingroup\$ Also if you add last weeks items in several places, this of course keeps the logic in one place. \$\endgroup\$ – Mongus Pong Feb 16 '11 at 21:56
  • \$\begingroup\$ If you are adding arbitrary times in just this place, then it probably isn't so useful.. \$\endgroup\$ – Mongus Pong Feb 16 '11 at 21:57
  • \$\begingroup\$ +1. +50 if I could. Abstraction is what it's all about, and you identified a very useful abstraction here. \$\endgroup\$ – Christoffer Lette Feb 21 '11 at 10:39
2
votes
\$\begingroup\$

This is what I would do:

shippedItems.AddRange(
   OrderItem.Fetch(market: this.MARKET,
                   shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
                   orderPlacedAfter: serverTime.AddDays(-7),
                   orderPlacedBefore: serverTime.AddHours(-85))
);

I feel that just as with brackets ({}) the closing parenthesis should be on its own line when a method call spans multiple lines. It seems more consistent to me.

Alternatively, you could line up the :'s:

market:           this.MARKET,
shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE

but that it difficult to maintain.

Another option:

shippedItems.AddRange(OrderItem.Fetch(market: this.MARKET,
                                      shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
                                      orderPlacedAfter: serverTime.AddDays(-7),
                                      orderPlacedBefore: serverTime.AddHours(-85)));

That uses fewer lines, but is longer horizontally. Also, at first glance it looks like the arguments are to AddRange, not Fetch.

\$\endgroup\$
  • \$\begingroup\$ I have been a fan in the past of the trailing ); as it makes a nice distinct "block" of code. But, I gave it up because I was ending up with multiple lines in a row of ) and }. For now, indent level is enough for me. \$\endgroup\$ – jphofmann Feb 14 '11 at 16:15
  • \$\begingroup\$ Also, if you were willing to give AddRange its own level of ); why not a ) on a single line for Fetch? \$\endgroup\$ – jphofmann Feb 14 '11 at 16:16
  • \$\begingroup\$ @jphofmann: It's a matter of style, really. My reasoning for placing OrderItem.Fetch on a new line was to prevent readers from thnking that the arguments were to AddRange. That does not apply in Fetch's case, so I left them on the same line. \$\endgroup\$ – Michael K Feb 14 '11 at 16:19
  • \$\begingroup\$ @jphofmann first comment: There are limits, obviously. However, I've found that many times if you have calls that go that long you really need to do some refactoring anyway. \$\endgroup\$ – Michael K Feb 14 '11 at 16:21
0
votes
\$\begingroup\$

I think you are unwise to reject creation of variable used only once. The forced line breaks you seem to be requesting are difficult if not impossible to implement in an aesthetically satisfactory way in an automated formatter - and if you're not using an automated formatter, you're wasting time.

var state = ORDERITEMSHIPCONFIRMSTATE.NONE;
var start = serverTime.AddDays(-7);
var end = serverTime.AddHours(-85);
var range = OrderItem.Fetch(market: MARKET, shipConfirmState: state, orderPlacedAfter: start, orderPlacedBefore: end);
shippedItems.AddRange(range);
\$\endgroup\$
  • \$\begingroup\$ What automated formatter would you suggest? \$\endgroup\$ – jphofmann Feb 20 '11 at 14:58
  • \$\begingroup\$ I'm afraid I'm from the Java world, not .net; I don't know what's available there, although I would hope Visual Studio would offer something like Eclipse's options for code formatting. If not, there must be some tool that will do the job. \$\endgroup\$ – Carl Manaster Feb 20 '11 at 16:04
0
votes
\$\begingroup\$

I really like Mongus Pong's answer, but I have a couple of things to add.

  • The name of the extracted method should reflect the actual meaning of the magic numbers (-7 and -85) inside the method.
  • The magic numbers themselves should probably be turned into named constants, for added clarity.
  • The extracted method should be put where it belongs. I would suggest using the repository pattern, but in this specific case it would probably be a good idea to put it next to the Fetch method, inside the OrderItem class. (The OrderItem class looks like a domain entity to me, so this would break SRP, but I digress.)
  • If all usages of Fetch could be replaced by methods like this, Fetch could eventually be made non-public.

Something like this:

shippedItems.AddRange(OrderItem.FetchLastWeeksOrderItems(MARKET, serverTime));

public class OrderItem
{
    public static IEnumerable<OrderItem> FetchLastWeeksOrderItems(
        string market, DateTime serverTime)
    {
        return OrderItem.Fetch(
            market: market,
            shipConfirmState: ORDERITEMSHIPCONFIRMSTATE.NONE,
            orderPlacedAfter: serverTime.AddDays(-7),
            orderPlacedBefore: serverTime.AddHours(-85));
    }

    private static IEnumerable<OrderItem> Fetch( ... )
    {
        ...
    }
}
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.