views:

494

answers:

18

My colleagues tell me that table based formatting of code is bad and it is no readable and I should follow conventions. What's that bad about table based formatting? Why is it banned?

I'm asking because for me it's much more readable.

Examples (not real code):

if (res == ResultType.Failure)               
  something = ProcessFailure(..);
if (res == ResultType.ScheduledAndMonitored) 
  something = DoSomething(...) && DoSomething3(..);
if (res == ResultType.MoreInfoAvailable)     
  info = GetInfo(..);
if (res == ResultType.OK && someCondition)   
  something = DoSomething2(..);
.... continued

versus

if (res == ResultType.Failure)               something = ProcessFailure(..);
if (res == ResultType.ScheduledAndMonitored) something = DoSomething(...) && DoSomething3(..);
if (res == ResultType.MoreInfoAvailable)     info      = GetInfo(..);
if (res == ResultType.OK && someCondition)   something = DoSomething2(..);
.... continued

Why I think the second one is better:

  • I don't need to parse the text by eyes - I see the structure of the commands at one glance.
  • I see immediatelly
    • that there are some ifs and assignments
    • that the enum used in conditions is ResultType and nothing more
    • that only the last condition is composed from two expressions

Update: this is not a real code. I just wanted to show some example. Consider it like it's an old code that somebody wrote some time ago and you need to read it. Why the first formatting is preferred over the second?

+2  A: 

I would say there are a few reasons: to reasonably be expected to main a layout like this you will need a IDE / plugin that will maintain this for you without this support there is a large on going effort that will be required for developers to be spaces and tabs wranglers all day long, the second and probably more likely reason is most code that would benefit substantially from this layout is "wrong". Your sample could most likely be written to use polymorphism that would be much cleaner and make this a moot point.

Chris Marisic
There is some plugin to VS2010 that allows that, so it's possible. Anyway, you're right, because one command can via re# can destroy the formatting completely.
stej
+5  A: 

The "table-based" layout you describe can be very useful in certain cases:

public string GetMessageFromErrorCode(int code)
{
    switch (code)
    {
        case 1: return "OK";
        case 2: return "Syntax Error";
        case 3: return "Other error";
    }
}

Note that this example is a valid exception to the c# rule that "All case statements must also include a break."

I am not fond of being rigid about code layout. Following a coding standard is a good thing. But as soon as you start doing things like using several parameters with long names in function calls, writing linq queries, or using anonymous methods or intricate lambda statements, it can make your code more readable to break the rules from time to time.

See Also:
http://stackoverflow.com/questions/2627662/anonymous-methods-lambdas-coding-standards

Robert Harvey
but doesn't that mean that there are just different rules for anonymous methods and lambda statements?
partkyle
@Kyle: I haven't seen a standard for that, although I suppose you could write one. All of the coding standards I've seen predate Linq, although interestingly, Visual Studio favors the convention that most people use (lining up the linq statements with the equals sign, or the dot operators). It's harder to come up with a standard for things like many parameters, anonymous methods, and long lambda statements, since it can be a judgement call to find a layout that is asthetically pleasing.
Robert Harvey
Small note: do you mean "All `case` statements must also include a `break`"?
Chris Burt-Brown
@Chris: Thanks, fixed.
Robert Harvey
+13  A: 

When your colleagues say you should follow conventions, does that mean your team has a formatting convention? If that is the case, that is enough reason right there. When everybody formats their code the same way, it is easier to read your teammate's code.

Peter Recore
This is a great point, and it has been described to me as follows: Which is the right side of the road to drive on? It doesn't matter, as long as everyone does it the right way.
funkymushroom
I think they ment conventions like '*nobody* formats the code like this' :) This is not established in our coding standards.
stej
@Peter your answer is absolutely correct of course, but it's not answering the OP's question IMO. -1
Pekka
@Pekka - The OP's question as written (especially as it was written when I answered it, before the update at the bottom) boiled down to "Why did my coworkers tell me I should follow certain conventions when I layout my code?" I feel my answer did address that question. The update changes the focus of the question, but I can only spend so much time at work monitoring SO :)
Peter Recore
@Pekka And even if the question is now "why is this layout not as preferred as this other layout", the answer "because everyone else does it this way" is still a valid answer. That is not to say I approve of doing something just because everyone else is doing it. But when it comes to aesthetic decisions like this, sometimes you just have to go with the flow.
Peter Recore
+5  A: 

The reason is simple.

If you are using spaces to indent everything, and make it lineup, it's a pain to keep it inline when a variable or condition changes. You have to manually go in and add/remove spaces.

If you are using tabs to indent everything, and make it lineup, it won't look the same on everyone's computer since not everyone has their tabs set to the same width. Some people have them set to 3 characters, others 5, others something different. And what looks nice and neat and "table like" on your machine, looks like a mess on theirs.

Throw braces around the statements that are executed by the if, and format it normally.

if (res == ResultType.Failure)               
{
  something = ProcessFailure(..);
}

if (res == ResultType.ScheduledAndMonitored) 
{
  something = DoSomething(...) && DoSomething3(..);
}

if (res == ResultType.MoreInfoAvailable)     
{
  info = GetInfo(..);
}

if (res == ResultType.OK && someCondition)   
{
  something = DoSomething2(..);
}

Oh, and the other reason is, not everyone uses a fixed with font for programming. Believe it or not, some developers use proportional fonts.

Chad
*If you are using spaces to indent everything, and make it lineup, it's a pain to keep it inline when a variable or condition changes* - even with a plugin to VS2010 they consider that bad style. So that's (I think) not the reason.
stej
@stej, no, of course it is. Not everyone will have that plugin. Why make the environment required to view your code more complicated than necessary?
Chad
+1  A: 

Hard to say what is going on in your sample, but it looks like you are doing a lot. If so, it to me it seems wrong that you want to cram so much logic into just 4 lines. This is C#, not Python. Speaking of C# vs Python, you are not using braces for if statement body, which MANY would not like. StyleCop would not like it either. Speaking of StyleCop, you also might want to run FxCop as well as ReSharper - it might suggest a cool way to improve the code. Why are you not using return? Even if the conditions are mutually exclusive, I find return very useful - allows to skip the rest of stuff without using lots of else if, yet still be able to detect a so called "default" case (think switch). My stab at it:

// This should be in it's own function; I am just too lazy to spell out a signature.
{
    if (res == ResultType.Failure)
    {
        return ProcessFailure(..);
    }

    if (res == ResultType.ScheduledAndMonitored)
    {
        bool result = DoSomething(...);
        result = result && DoSomething3(..);
        return result;
    }

    if (res == ResultType.MoreInfoAvailable)
    { 
         info = GetInfo(..);
         // Ok, and then?
    }

    if (res == ResultType.OK && someCondition)
    { 
        return DoSomething2(..);
    }

    // Else ...

    Debug.Assert(false, "Why did we get here? Explanation: ");
}

What I wrote above is not the best code, but I tried to clean it up. Do not worry about extra variables - the optimizing compiler will take care of them. Do go for logical readability. In my strong opinion, putting things into tabular format does not help. If code is too hard to read once StyleCop is happy, then you probably need to re-factor it, as well as simplify logic/state/etc.

Without know exactly what you are trying to do, it is hard to give a better code sample.

Hamish Grubijan
+1  A: 

Focusing in on the example given, I would format it like this. I find both your examples hard to read.

if (res == ResultType.Failure)               
{
  something = ProcessFailure(..);
}
if (res == ResultType.ScheduledAndMonitored) 
{
  something = DoSomething(...) && DoSomething3(..); //Bit-and on the results? huh.
}
if (res == ResultType.MoreInfoAvailable)     
{
  info = GetInfo(..);
}
if (res == ResultType.OK && 
    someCondition)   
{
  something = DoSomething2(..);
}

But, supposing this was an actual snippet, I would have written it as a switch/case sort of thing. Possibly with careful use of dropping down from the previous break. :o

Paul Nathan
+3  A: 

The problem that I see with your example is that the eye connects the information vertically, for instance, when I glanced at the code I saw

something
something
info
something

And mentally grouped them, where as in the other, I saw the flow and logic of the code and then the details. This makes it more straight forward to cram what I need to into my head.

ilivewithian
*...the eye connects the information vertically* - that is what I consider as advantage. I can see immediatelly affected variable without the noise (conditions) around.
stej
Yes, but what happens then is you end up code myopia, rather that the more important flow of the code.To some extent it's a preference thing, but the overriding call must be following the already set standards.
ilivewithian
A: 

It's not more readable to all of us. Imagine if regular English text had strange tabulations in it to line up the subjects and predicates of sentences. That'd be hard for the eye to follow, no? I read code much like I read prose, so when I have to jump around horizontally a lot, it's a strain to me. However, using tabs when initializing data structures is helpful since that's more-logically a table to me.

So that's why code formatting standards are important; there's no one true way to best format code that will please everyone.

Jacob
+2  A: 

I think you have a good case for making an exception to the formatting rules, but my opinion won't help your cause. My true advice is if you are a team and multiple people are telling you to do it one way but you want to do it another, and you stand to lose nothing but pride, it's more productive to move on, gaining a tiny bit of respect as a team player, which might turn into the kind of thing that lets you get your way in the future.

I learned the above by not following it most of my carrear - it is just not worth the headaches sometimes.

Gabriel
A: 

Consider the case of parameter validation and possible early exit in a function. Sometimes I like to format my code similar to the original example to reduce the validation code in relation to the rest of the code. I could be validating the input prameters or I could be checking the validity of hte requested operation based on the internal state of the object. For example:

void LogMessage(LogLevel lvl, string msg)
{
  if (!this.loggingManager.IsLogging) return;
  if (lvl < this.loggingLevel) return;

  this.loggingManager.Write(this.loggerName, msg);
}

Or

void DrawGeometry(Graphics g, Geometry geom, Layer layer, double scale)
{
  if (geom == null) return;
  if (!layer.IsDisplayed) return;
  if (this.isDisplayByScale && (scale < this.minScale || scale > this.maxScale) return;
  if (this.ColorsToTreatAsTransparent.Contains(layer.Color)) return;

  Geometry xformed = this.Transform(geom);
  using (Pen p = new Pen(layer.Color, layer.Width))
  {
    g.Draw(xformed, p);
  }
}

Note that both of these are completely made up examples. I go back and forth, but I do kind of like the brevity of the one line sanity check/return statements.

wageoghe
For the second example, you might want to re-factor this all into a function which returns bool. The optimizing compiler should do a good job at inlining.
Hamish Grubijan
+2  A: 

I think it depends a lot on the personal choice as well as to what formatting to use. In a professional environment you would expect the team to have a defined and agreed coding standard which all the team members follow.

There are many tools and add ins to IDE's like Visual Studio which give you the capability to suggest changes to the code structure by parsing the source code files.

One such too which i find very useful is the [StyleCop][1].

I personally find Stylecop very useful to have a consistant coding guideline.

Regards, Nilesh

Nilesh Gule
+1  A: 

If you feel the need to tabularize your code, it's the right time to think. It smells like refactoring time. Code should be readable like a sentense.

Second reason is the code maintability cost. Tabularized code is harder to maintain.

rarouš
+1  A: 

Even though I also find your second (table-like) example more readable, I would not use it because

  1. This kind of layout is not easy to maintain (you'll possibly have to go through the whole list if one line changes). Therefore it's highly probable that someone will eventually be too lazy to adjust it and the layout will become an unreadable mess.
  2. If a single line has, say, three or four conditions, all lines will become unnecessarilly long and therefore unreadable.
  3. And of course, if it's against the coding standard in your team, please just don't do it :) ...
MartinStettner
A: 

Curly braces to create statement blocks are just more common industry wide, build the habit now because most people will think your code looks dinky if you write it like that.

On another note, one of the reasons I've seen the rule held that you always use curly braces for if statements and never do an if (condition) single-line-statement; is because it's not uncommon for a line of code to change 4 months later causing the if statement to fall through to the next statement without you realizing it, therefore as safety, if's should always be:

if (condition)
{
    single-line-statement;
}
Jimmy Hoffa
I know about the problem with braces. But personally I don't consider this as a real 'problem'. a) you see where you put your statement, b) if you go behind the `singel-line-statement` and press Enter, code editor places the cursor correctly (on the column where `if` begins). So you see you are not in the block.
stej
A: 

It would be nice if programming languages, markup languages, and text editors could all cooperate with each other to allow text formatting to adapt itself sensibly to fit the code. For example, if a comment associated with a line of code would be too long to fit to the right of that line, and if succeeding lines of code are shorter, allow the comment to wrap and remain to the right of those succeeding lines of code, with some sort of arrow or box to show that the continuation lines of the comment all belong to the first line of code.

Unfortunately, I know of no widespread programming languages which fit well with any means of embedding markup.

supercat
+1  A: 

The key to readability is consistancy. If the same construct appears different ways at different times, then it will be more difficult to process mentally. Therefore, I would not like to maintain C# code that was formatted in a tabular style.

Also, tabs lie.

if (!HydrogenTankFull())                          FillHydrogenTank();
if (!OxygenTankFull())                            FillOxygenTank();
if (HydrogenTankFull() && OxygenTankFull())       TestGimbals();
if (GimbalsTested() && LaunchButtonPressed())     IgniteEngine(); BlowRestrainingBolts();
if (LaunchPadCleared())                           TransferControlToHouston();

Why did the rocket just fall over?!

Jeffrey L Whitledge
I'm aware of the problem with braces. You can add them to the formatting ;). I admit, your code reds bad, don't know why. Maybe too much space.
stej
+2  A: 

I think your "table-based" example is far easier to read, on its own. The problem is that we've got 50 years of text-based tools for dealing with source code as lines of text. The two most obvious problems that you'll run into with this are:

  1. All my favorite text-handling tools, like diff and grep, work on lines. The more you put on one line, the less value my tools have. For example, if I want to grep for something in the if's condition, I have to filter false positives from the ifTrue clause.

    (If you're writing in a language with more regular syntax in an extensible editor, like Lisp inside Emacs, this isn't as much of a problem, since it's trivial to traverse the actual structures. But C# has very complex syntax, and I don't know of an easy way to traverse my C# code with 1 or 2 lines of code, so text tools still reign here.)

  2. Your longest line is about 95 characters -- and that's with .. in place of actual args, so it'd be even longer. Really long lines can be a pain to work with. Lines of varying width (since most of my lines won't be that long) mean wasted screen space, since my windows are all rectangular. Or I have to keep resizing windows when I scroll. Any way I look at it, editors and tools are great at handling files with many lines, but pretty lousy at handling lines of wide and varying lengths.

I'd like writing this code. I'd love reading this code. I would really hate having to do a 3-way merge on this code. :-)

In contrast, I'm going to go out on a limb and say I think most of the other answers here so far are bunk:

  • various things about char 0x09: well, use spaces and not tabs, then; that's just common sense these days
  • monospaced fonts: all kinds of code standards I've seen fall over if you use a variable-width font, not just this
  • "it's the convention": OK, but that doesn't say why it's the convention, or why this case shouldn't be an exception to the convention (because every set of code conventions I've ever seen allow exceptions)
  • StyleCop says it's bad: are you working for your tools, or are they working for you? see also: "it's the convention"
  • it's hard to maintain: not in my editor, and if it's hard in your editor, you need to learn how to use it, or get a better one
  • some people won't have your editor: do all your coding standards depend on it being easy to write with an arbitrarily wimpy editor? If we hire somebody who insists on using Notepad, that's their problem, and we're not going to change our source code for them. See also: "it's hard to maintain"
  • it's inconsistent: sure, but (since I'm not a robot) that doesn't necessarily make something harder to read; every comic strip in my newspaper is formatted differently (in some cases really differently!), yet I don't find any of them at all difficult to read

(Flame away!)

In the end, I think both of these styles are kind of lousy for this kind of code, for different reasons. If you need a table of stuff, then use switch, or an IDictionary<ResultType,Action>, or a list-of-lists or -tuples (if order matters), or offload the rules into a config file, or something. You're right to think that a table should look like a table. The only thing you're missing is that we've got constructs to make things look like tables that would work just fine here, and if isn't one of them. :-)

Ken
Were you looking into my head when writing the response? :) What you type about conventions, StyleCop etc. was what I thought. That thing with diff is also interesting (I use WinMerge that is able to show differences on letters).
stej
+1  A: 

In otherwise top-down code flow, your brain has to switch to the "table reading mode" for this code block. And make sense of what the table "columns" are. And recognize the end of the table block and switch to the normal reading mode.

Too much hassle IMO.

Borek