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).
2
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.
5
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.
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.
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.