Code Quality – Keep It Simple Now or Program with the Future in Mind?

It makes sense that your developers want to see their changes as soon as they make them. Much like you typically build and run unit tests before you check in (or merge upstream), it's a good idea to run static analysis before you check in (or, again, merge upstream). They all serve the same high-level purpose: make sure that you didn't introduce any new problems or reintroduce an old problem.

I don't know how long it takes to build your system, but some organizations have success with running a build and test on every check in. If you have a team that is frequently checking in or merging or if the build/test/analyze cycle takes too long, this might not work for you. In addition, like you mentioned, it reduces the "compare since last build" functionality since I would suspect that many of the check ins and builds would be fairly small changes.

I think the best option would probably be to allow developers to run SONAR locally as part of their pre-commit build/test/merge process. This would be especially helpful if they are targeting a particular SONAR-identified problem. Prior to checking in and merging, a developer can be sure that they have fixed the issue they intended to without introducing new (or higher priority) problems. This would also prevent cluttering your nightly build environment with a large number of builds - each build should be reflective of a day's work for your team.

As an exercise, first let's verify your logic. Though as we'll see, you have bigger problems than any logical problem.

Call the first condition A and the second condition B.

You first say:

Looking specifically at section two, I know that if section one is true, then section two will also be true.

That is: A implies B, or, in more basic terms (NOT A) OR B

And then:

I can't think of any reason as to why section one would be false and section two evaluate true, which makes the second if statement redundant.

That is: NOT((NOT A) AND B). Apply Demorgan's law to get (NOT B) OR A which is B implies A.

Therefore, if both your statements are true then A implies B and B implies A, which means they must be equal.

Therefore yes, the checks are redundant. You appear to have four code paths through the program but in fact you only have two.

So now the question is: how to write the code? The real question is: what is the stated contract of the method? The question of whether the conditions are redundant is a red herring. The real question is "have I designed a sensible contract, and does my method clearly implement that contract?"

Let's look at the declaration:

public static CalendarRow AssignAppointmentToRow(
    Appointment app,    
    List<CalendarRow> rows)

It's public, so it has to be robust to bad data from arbitrary callers.

It returns a value, so it should be useful for its return value, not for its side effect.

And yet the name of the method is a verb, suggesting that it is useful for its side effect.

The contract of the list parameter is:

• A null list is OK
• A list with one or more elements in it is OK
• A list with no elements in it is wrong and should not be possible.

This contract is insane. Imagine writing the documentation for this! Imagine writing test cases!

My advice: start over. This API has candy machine interface written all over it. (The expression is from an old story about the candy machines at Microsoft, where both the prices and the selections are two-digit numbers, and it is super easy to type in "85" which is the price of item 75, and you get the wrong item. Fun fact: yes, I have actually done that by mistake when I was trying to get gum out of a vending machine at Microsoft!)

Here's how to design a sensible contract:

Make your method useful for either its side effect or its return value, not both.

Do not accept mutable types as inputs, like lists; if you need a sequence of information, take an IEnumerable. Only read the sequence; do not write to a passed-in collection unless it is very clear that this is the contract of the method. By taking IEnumerable you send a message to the caller that you are not going to mutate their collection.

Never accept nulls; a null sequence is an abomination. Require the caller to pass an empty sequence if that makes sense, never ever null.

Crash immediately if the caller violates your contract, to teach them that you mean business, and so that they catch their bugs in testing, not production.

Design the contract first to be as sensible as possible, and then clearly implement the contract. That is the way to future-proof a design.

Now, I've talked only about your specific case, and you asked a general question. So here is some additional general advice:

• If there is a fact that you as a developer can deduce but the compiler cannot, then use an assertion to document that fact. If another developer, like future you or one of your coworkers, violates that assumption then the assertion will tell you.

• Get a test coverage tool. Make sure your tests cover every line of code. If there is uncovered code then either you have a missing test, or you have dead code. Dead code is surprisingly dangerous because usually it is not intended to be dead! The incredibly awful Apple "goto fail" security flaw of a couple years back comes immediately to mind.

• Get a static analysis tool. Heck, get several; every tool has its own particular specialty and none is a superset of the others. Pay attention to when it is telling you that there is unreachable or redundant code. Again, those are likely bugs.

If it sounds like I'm saying: first, design the code well, and second, test the heck out of it to make sure it is correct today, well, that's what I'm saying. Doing those things will make dealing with the future much easier; the hardest part about the future is dealing with all the buggy candy machine code people wrote in the past. Get it right today and the costs will be lower in the future.