automatically add curly brackets to all if/else/fo

问题:

I want to reduce the number of sonar violations in a large legacy java code-base, and it seems a "quick win" would be to update all these conditional statements to have curly brackets. This seems like an easy thing to do and I can't see why it wouldn't be readily automatable.

Does anybody know of a tool that could perform a bulk operation like this? Or why to do such a thing might be a bad idea before I go and spend the time writing something myself? If I were to write one myself what would be the best tools to use? Ideally something that is java language aware, so that I don't have to deal with formatting corner-cases and the like.

The rule is non-negotiable by the way, so this really is the best approach.

回答1:

The easiest thing would be to use Eclipse and click Clean-up on the whole project. In Clean-up profile configuration select Code style tab. There you can select Use blocks in if/while/for/do statements as Always.

回答2:

First enable Control flow statement without braces in the inspection settings.

IntelliJ Idea -> Run Code Inspection -> Quick Fix (works at least in the commercial version)

回答3:

While it is advisable to be cautious with legacy code, it is also a good thing to detect bugs in legacy code ... or at least make the bugs easier to spot.

Let us consider Brian Agnew's difficult cases:

// Case #1
if (x) doMethodA(); doMethodB();

In fact, as far as the JLS and the Java compiler are concerned, this means

if (x) doMethodA();
doMethodB();

So when the transformer rewrites the code to:

if (x) { 
    doMethodA();
}
doMethodB();

it is not changing the meaning of the code, but it is correcting a problem that is likely to cause someone to misread the code, and miss a potential bug that is in the code already; i.e. if the 2nd call is supposed to be conditional ...

// Case #2
if (x) 
    // doMethodA();
    doMethodB();

Once again, when that is rewritten, you should get:

 if (x) {
    // doMethodA();
    doMethodB();
 }

which means the same thing as the original. Furthermore, this most likely reflects the intent of the programmer ... if the indentation is to be believed. But consider this:

// Case #2a
if (x) 
    // doMethodA();
doMethodB();

When we rewrite that as

if (x) {
    // doMethodA();
    doMethodB();
}

the actual meaning of the code doesn't change, and the incorrect indentation is no longer going to mislead. If the programmer decides to uncomment the first call, he might not realize that the previous commenting out had had an unintended consequence. (The evidence was in the original indentation that we have "fixed".) But there is a potential solution; see below.

---

If we assume that the code transformation tool operates with a proper understanding of the syntax and semantics of Java, then it will no break anything that wasn't already broken, and it will (to a degree) make any existing brokenness more obvious to someone reading the code. To me, that is a zero-risk win, even for legacy code.

Now if we make the transformer smarter, it could detect some of the cases where the original indentation indicates a possible bug (like cases #1 and #2a above) and flag them for closer code inspection.

回答4:

This strikes me as potentially quite dangerous. Do you have comprehensive unit test coverage ?

I can see some immediately difficult cases e.g.

if (x) doMethodA(); doMethodB();

and

if (x) 
  // doMethodA();
  doMethodB();

are two that spring to mind and can't be naively handled (you should be language-aware to tackle these). I wouldn't try and automate such a task unless you have language-aware and reliable tool, but rather enforce a policy that you amend code when you're changing it for other reasons, and can at that stage assert code coverage through unit tests prior to changing the code.

回答5:

Robert in a comment said: "@ira i would also be interested in hearing of this truly language aware tool!"

Sorry for the tease. For the cause of the tease, check my bio.

Untease: see http://www.semanticdesigns/Products/DMS/DMSToolkit.html as the foundation engine, and http://www.semanticdesigns.com/Products/FrontEnds/JavaFrontEnd.html. The pair make a fully language aware source-to-source program program transformation system.

One would accomplish this task with DMS with two source-to-source transformation rules:

  rule curlies_for_then_statement:(c:expression, s: statement}: 
             statement->statement
       " if (\c) \s " -> "if (\c) { \s } ";


  rule curlies_for_else_statement:(c:expression, b: block, s: statement}:
              statement->statement
       " if (\c) \b else \s " -> "if (\c) \b else { \s } ";

This works reliably because DMS is driven completely from a formal grammar (for Java in this case), and is not operating on raw text, but rather abstract syntax trees; this also makes in independent of layout. (It is probably useful to understand the rules that "block", "expression" and "statement" are grammar nonterminals.

回答6:

While it may be a quick sonar win, you are doing some dangerous stuff. The recommended approach is to only fix legacy code when that code has to be revisited. These blanket approaches are going to potentially cause very subtle bugs. Because policy has changed your management team needs to understand that this is a massive undertaking, and should guarantee that all future code is developed to the standard.

回答7:

Intellij 2017 support this with Reformat Code, if you set the right coding style in the editor settings first.

File -> Settings -> Editor -> Code Style -> Java -> Wrapping and Braces

回答8:

The rule is non-negotiable by the way, so this really is the best approach.

Interesting.

You have a legacy codebase that is full of style errors, AND it is not negotiable that the style errors must be fixed.

The value proposition of this exercise is (to say the least) dubious. "Legacy codebase" is often a polite way of say that the code ought to be thrown away and rewritten ... but you can't afford it. However, apparently, you can afford to spend correcting missing braces, which is unlikely to make any real difference to maintainability.

As previous answers point out. If you automate this style correction properly:

• It won't actually fix any bugs but it won't introduce any new ones either.

• It might remove "clues" (e.g. dubious indentation) that are would be flags for existing bugs.

But if you do the style corrections manually:

• There is a chance that you will notice some bugs. These can be added to your issue tracker for further diagnosis and fixing.

• There is also a chance that you will introduce new bugs. And if we assume that the existing unit / systems are non-existent or of low quality (this is a legacy codebase) then these new bugs have a fair chance of going undetected.

Now generalize across style-related errors that Sonar detects.

In summary, this whole process of fixing all of the style errors in a legacy codebase is (IMO) largely a waste of time if automated, and possibly risky if manual.

This should be negotiable. You should be able to tell management of these concerns ...