Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Our development process is as follows

code the task -> someone else QAs code and documentation -> task is merged into trunk.

Recently a colleague is refusing to pass the code QA due to issues with indentation and whitespace.

Here are examples of these issues (syntax is SAS):

Additional whitespace:

        %if &syserr gt 0 %then %goto err; /*last line of code*/




/* Footer area*/

Extra line of white space, and not indented inside proc sort:

 /* End Of header * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */


    proc sort data = %dataset ;
    by id; 
    run;
    %if &syserr gt 0 %then %goto err; 

    proc sort data = &second_dataset.;
    by id;
    run; 
    %if &syserr gt 0 %then %goto err; 

Extra white space between steps:

        /*join all details on for each record*/
        proc sort data = &data out = data_srt ; 
              by &conditions; 
        run;
        %if &syserr gt 0 %then %goto err;

        proc sort data = &data2.;
              by &conditions.;
        run; 
        %if &syserr gt 0 %then %goto err; 




        /*cartesian join*/
        data new_data;
              join data 
                          &data2.  ;
              by &conditions; 

        run;

The question is, being a good programmer, is looking over your code and correcting all this kind of thing the right thing to do, or is this just ridiculous?

There is an additional complication, that because we don't have continuous integration or automated testing, it's not possible for the QAer to quickly fix up these issues and commit the code, for risk of accidentally deleting semicolon or something. (To be fair, the risk applies to the initial developer making these changes, so either way if this mistake occurs, it just needs to be fixed and move on).

share|improve this question

closed as primarily opinion-based by gnat, GlenH7, MichaelT, Dan Pichelman, Dynamic Nov 29 '13 at 4:49

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

1  
I don't know SAS but extra whitespace is pretty anal. Indentation strikes me as a fair point. –  Erik Reppen Nov 26 '13 at 4:53
    
@ErikReppen On the other hand, those examples have 4 blank lines between code segments. That's kind of excessive... –  Izkata Nov 26 '13 at 5:32

4 Answers 4

up vote 14 down vote accepted

Yes, that's the right response. The indentation style should be consistent for all code.

A big part of the value of consistent indentation is that it's consistent. That way people learn to read it easily, which speeds up everyone.

My rule of thumb is that any indentation style the team wants is good, as long as it can be mechanically applied. Applying it mechanically means that you don't have to learn the details of the standard, or spend time twiddling whitespace.

Mechanical formatting also becomes another way that errors stand out. If you think you've enclosed code in a block the format tool will un-indent it and it'll be obvious that you messed up. This also makes refactoring easier - at the trivial level when you extract a block of code to a function, autoformat will remove the extra indentation.

Autoformat also means that if you really, really can't live with the standard everyone else uses you can format code the way you like it, then reformat it back before you check it in. Since you have a review step, you'd obviously do that before the review and if you didn't it would be picked up then.

GNU indent is one tool that can be made to autoformat almost anything. I did a quick search and there seem to be tools around to auto format SAS code but the SAS tools don't do that for you.

share|improve this answer
6  
+1 for using tool. It is kinda ridiculous to fix it by hand when you can just run a tool in commit hook. –  imel96 Nov 26 '13 at 5:04
1  
@imel96 Sometimes there is intentional whitespace to aid readability, that an automatic tool will screw up. So it should be optional, something you can trigger when you want it, not forced with each commit. –  Izkata Nov 26 '13 at 5:34
    
@Izkata: I've yet to see a formatting tool that doesn't have a "don't format this bit" code you can use. But I've also gone quite a long time since I last wanted to disable autoformatting. I admit to occasionally using slightly odd code constructs to get the format I want (multiline conditionals with "if true and" on the first line, for example). But the multiline conditional is a code smell all by itself, so adding a bit to make it less unreadable is IMO fair enough. –  Ӎσᶎ Nov 26 '13 at 7:03
    
@Izkata, the problem with making the tool optional is that soon as one person opts out for a file, no-one else can ever run the tool on that file again or they will break the "don't format this file" code. Which means they should rightfully flick that file to you and say "fix the formatting". Every time it gets modified. So no, optional does not work. You need to use the no-format codes around your special blocks of code/comment. –  Ӎσᶎ Nov 28 '13 at 9:38
    
@Ӎσᶎ So you fix the indentation that's actually bad. Don't blindly run it over the whole file. vim, for example, allows you to select a chunk of the file and automatically reindent that, ignoring the rest of the file. –  Izkata Nov 28 '13 at 15:28

This is absolutely the correct thing to do. One of the biggest parts of code quality is its readability. If you haven't indented your code properly and you have random whitespace everywhere it reduces the readability of the code.

Generally, your development team should all follow the same code quality standards when it comes to indentation and whitespace. If other modules in your code base don't put extra whitespace between steps then this one shouldn't either (unless of course, it improves readability).

I'm not a SAS programmer, but if I'm reviewing code and the indenting is all out of whack and it doesn't look tidy I certainly comment on it for follow up and, if really bad, fail the review.

share|improve this answer

As Uncle Bob says in his book, formatting (even whitespace) is incredibly important. Software tends to be read much more often than written, so it behooves us to make it clean and easy to read. It's a method of communication.

Ideally, when working in a team, you'll determine a standard and everybody follows it. Ideally, instead of nitpicking the standard in individual code reviews, set up a tool that will do the job for you. (Not sure what tool would work for your case; something like checkstyle for Java or StyleCop for C#).

So I'd chat with your colleague and see if you can't come up with some guidelines about whitespace. Taking a couple minutes to clean up your code is worth it if it makes things consistent down the line.

share|improve this answer

After an experience many years ago, reading some F-16C/D code where the indentation was broken, I have to say that getting the indentation correct is critically important.

It is so critically important that it should not be done by hand and it should not be fixed by hand. This is what computers are for.

They make computer programs that can automatically reformat source code to match your company's preferred style. Hook one of them up to your source code control system, so that the code is automagically brought into compliance whenever it is checked in.

share|improve this answer

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