Please question the need for whitespace

I have blogged about this before but I think it is a common problem that is worth restating since it affect developers across our industry.  I noticed the following method recently and again the curious separation of sections by whitespace popped into my head:

   1:  private void CalcHeaderOffsets()
   2:  {
   3:      this.fs = new FileStream( assemblyPath, FileMode.Open, FileAccess.Read );
   4:      this.rdr = new BinaryReader( fs );
   5:      dos_magic = rdr.ReadUInt16();
   6:      if ( dos_magic == 0x5a4d )
   7:      {
   8:          fs.Position = 0x3c;
   9:          peHeader = rdr.ReadUInt32();
  10:          fileHeader = peHeader + 4;
  11:          optionalHeader = fileHeader + 20;
  12:          dataDirectory = optionalHeader + 96;
  13:          // dotNetDirectoryEntry = dataDirectory + 14 * 8;
  14:   
  15:          fs.Position = peHeader;
  16:          pe_signature = rdr.ReadUInt32();
  17:          rdr.ReadUInt16(); // machine
  18:          numDataSections = rdr.ReadUInt16();
  19:          fs.Position += 12;
  20:          optionalHeaderSize = rdr.ReadUInt16();
  21:          dataSections = optionalHeader + optionalHeaderSize;
  22:   
  23:          sections = new Section[numDataSections];
  24:          fs.Position = dataSections;
  25:          for( int i = 0; i < numDataSections; i++ )
  26:          {
  27:              fs.Position += 8;
  28:              sections[i].virtualSize = rdr.ReadUInt32();
  29:              sections[i].virtualAddress = rdr.ReadUInt32();
  30:              uint rawDataSize = rdr.ReadUInt32();
  31:              sections[i].fileOffset = rdr.ReadUInt32();
  32:              if ( sections[i].virtualSize == 0 )
  33:                  sections[i].virtualSize = rawDataSize;
  34:   
  35:              fs.Position += 16;
  36:          }
  37:      }
  38:  }

What is the purpose of the new lines at 14, 22 and 34?  Are these separate blocks of logic?  Could they be better represented using "Extract Method"?

One of the first things that I do with code when refactoring is remove all the line breaks and make sure I am comfortable with the logical flow of the code.  If lines don't logically flow together then I ask myself if the two pieces belong next to each other or if they should be different responsibilities in different methods.

Ask yourself next time you leave a new line whether it is really necessary or is it a codesmell indicating poorly separated unrelated functionality?

FYI:  The code above is taken from NUnit (AssemblyReader.cs) and is free software licensed under the NUnit license (Copyright 2007, Charlie Poole, http://nunit.org/?p=license&r=2.4).

 

We are hiring!  Do you want to write beautiful code in a Test Driven, Refactored, Agile .NET software company in the heart of Washington DC and work on cool products? 
Take the code test and send your resume along with why you want to join Thycotic to tddjobs@thycotic.com.

11 Comments

  • adding empty lines is like using commas and periods in your sentences if you don't the text will be much harder to read readability isn't improved by extracting every 5-10 lines into methods with parameters going back and forth

  • You should compare it with reading a book. A book without paragraphs reads badly and you will most likely to miss a lot of things. However a badly paragraphed book, isn't good either as you will loose the connection between lines and it won't get clearer either.

    White spaces are a good thing, if used properly. And adding a whitespace is definatly NOT just a sign that you could extract a new method from it.

  • I agree with the above comments. It makes the code readable.

    I often add empty lines in logical places whenever I have to change some code I didn't write myself.

    It always easier to read code you've written yourself... think about your coworkers.

  • I think whitespace is essential to make code readable. Leaving space helps the cognitive process because the mind can easily form groups of code chunks and recognize sections at a glance.

    It's basic Gestalt theory at work.

  • Sometimes extracting a method does not make things more clear because the method needs to have lots of parameters and returns.

    Often, the human who is writing the code feels that adding a little space makes it easier for him/her or the next human to understand the code. The machine has no problem ignoring the space.

  • Well - I don't agree with the thought of removing all white space.

    It does not get in the way of the compile process and it does not necessarily designate a break in code flow. In the code example you show - I am not sure I would have the line breaks where they are - but I sure would not remove them willy-nilly.

    I teach my students to use white space consistently and where it will help to organize. I could be wrong, but I think this is the point you were trying to make.

    I write code for humans and giving the eye a break as you scan enables you to focus on successive chunks. Writing code is about organizing and certainly breaking code out into other methods is one possible way to organize - but not always.

    A good example I can pass along from my own daily work - I add a break every 5 lines when I am setting a significant number of properties or parameters. If you have data tables with 30, 50, 100+ columns in it and you are performing insert/update - setting those values in scrunched up blocks of text - 30, 50, 100+ straight lines of code just causes the eyes to go cross-eyed as you are scanning through.

    I have learned from designers who work in a variety of media (e.g. print, web, film, etc) - and from that I have gained valuable knowledge in what makes for easy reading. White space plays a significant role in that.

    I can tell you from reviewing a lot of student code and those of my co-workers... Code written for humans is easy on the eyes. Being able to view in chunks is faster to scan, and comprehend, and enables me (at least) to spot inconsistencies.

    Use it wisely.

  • Thanks everyone for your feedback. It has been an interesting discussion.

    I still think you will find less dependence on whitespace if you make your methods shorter and about one thing. :)

  • Interesting how a single exclamation point throws you off in this post "What makes some code confusing?" but having all of your code compressed into having no whitespace is easier for you?? That doesn't make much sense. White space != code smell. Extract method != prettier code.

  • @Snacks: Your comment is somewhat of an oversimplification and doesn't really add much else to comment on. I would encourage you to discuss and explain rather than simply make statements.

  • Jens:
    &gt; adding empty lines is like using commas and periods in your sentences if you don't the text will be much harder to read readability isn't improved by extracting every 5-10 lines into methods with parameters going back and forth
    I second this and think nothing more there is to add on the matter other than even more explicit critics.
    READABILITY is what we need and the number 1 key factor in any decent SE book. The rest is just misguiding and misinformation, including all the buzz on that logical nonsense called "refactoring" and the whole Agility bandwagon.
    Those guys are simply reinventing the wheel, and they are reinventing it wrong, because they didn't bother to check what had already been done. They have their "time to market" before all.
    -LV

  • @LudovicoVan: The only reason your comment wasn't originally published was that CommunityServer tagged it as "Possible Spam". I don't censor opinions, however I don't think your comment contributed much.

    Bashing refactoring without any real explanation doesn't come off as enlightened.

    The number #1 goal of refactoring *is* "READABILITY" as you put it.

    According to Bob Martin, code should do 3 things:
    * should work (hopefully this means that your unit tests pass)
    * should be easy to change
    * should be easy to read and communicates its purpose clearly

Comments have been disabled for this content.