Fix quality flaws 3

[guwirth]

This change is 

[ivangalkin]

Reviewed 82 of 82 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @guwirth)

---

cxx-checks/src/main/java/org/sonar/cxx/checks/MagicNumberCheck.java, line 111 at r1 (raw file):

String n

does SonarQube suggests to write final String n? because it would the logical continuation of your change IMHO

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/coverage/BullseyeParser.java, line 80 at r1 (raw file):

  public void processReport(File report, final Map<String, CoverageMeasures> coverageData)
    throws XMLStreamException {
    if (LOG.isDebugEnabled()) {

it's ok to write that for the sake of consistency, but imho this additional if harms the readability and has absolutely no effect

I believe, that the main purpose for such if (LOG.isDebugEnabled()) is to avoid computation overhead of traced objects. See example https://garygregory.wordpress.com/2015/09/16/a-gentle-introduction-to-the-log4j-api-and-lambda-basics/

Since this logging invocation doesn't have and calculated parameters (actually it doesn't have any parameters at all), the isDebugEnalbed()was not necessary here

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/coverage/TestwellCtcTxtParser.java, line 83 at r1 (raw file):

·····

trailing whitespaces

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/valgrind/ValgrindStack.java, line 56 at r1 (raw file):

  @Override
  public String toString() {
    StringBuilder res = new StringBuilder(256);

instead of streams + StrringBuilder one could use StringJoiner here

return frames.stream()
     .map(frame -> frame.toString())
     .collect(Collectors.joining("\n"));

---

cxx-squid/src/main/java/org/sonar/cxx/CxxConfiguration.java, line 109 at r1 (raw file):

    if (!allDefines.contains(value)) {

this check is contra productive, because Set will check the existency inside of the Set.add() invocation.

I believe, that the best solution is to write something like that:

public List<String> getDefines()
{
   Set<String> allDefines = new HashSet<>();
   for (Set<String> elemSet: uniqueDefines.values()) {
      allDefines.addAll(elemSet);
   }
   return new ArrayList<>(allDefines);
}

In general I don't really like the 1-to-1 rewriting of loops into streams. We miss the procedural approach completely (where are maps, filters, collects etc?) and make the readability worst.

---

cxx-squid/src/main/java/org/sonar/cxx/CxxConfiguration.java, line 150 at r1 (raw file):

    List<String> allIncludes = new ArrayList<>();

    uniqueIncludes.values().stream().forEach((elemList) -> {

again, this 1-to-1 nested loop to nested streams refactoring looks not really elegant.
I believe in many cases of nested loops you might want to flatten the streams

https://www.baeldung.com/java-flatten-nested-collections

together with destinct() and collectors, the stream implementation can become really easy

return uniqueIncludes.values.stream().flatMap(Collection::stream).distinct().collect(Collectors.toList());    

demo https://ideone.com/ZefJdQ

---

cxx-squid/src/main/java/org/sonar/cxx/CxxConfiguration.java, line 242 at r1 (raw file):

    List<File> files = new ArrayList<>();

    compilationUnitSettings.keySet().stream().forEach((item) -> {

same here: the stream-conform implementation looks rather as following

return compilationUnitSettings.keySet().stream().map(File::new).collect(Collectors.toList());

demo https://ideone.com/fvnm3z

---

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java, line 146 at r1 (raw file):

          LOG.debug("parsing external macro: '{}'", define);
        }
        if (!"".equals(define)) {

here and below the empty-define check looks strange.

was if (!define.isEmpty()) ment? or maybe if(define != null && !define.isEmpty())?

---

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java, line 198 at r1 (raw file):

  private static String serialize(List<Token> tokens, String spacer) {
    StringJoiner js = new StringJoiner(spacer);
    tokens.stream().forEach((t) -> {

probably...

return tokens.stream()
     .map(t -> t.getValue())
     .collect(Collectors.joining(spacer));

... would be more stream-style

---

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java, line 449 at r1 (raw file):

    List<Token> params = new ArrayList<>();
    identListAst.getChildren(IDENTIFIER).stream().forEach((node) -> {
      params.add(node.getToken());

same her: map + collectors will be more stream-like

https://docs.oracle.com/javase/8/docs/api/java/util/stream/Collectors.html

---

cxx-squid/src/main/java/org/sonar/cxx/preprocessor/CxxPreprocessor.java, line 1265 at r1 (raw file):
the short version of this code...

Quoted 4 lines of code…

      if (line != that.line) {
        return false;
      }
      return (path == null) ? that.path == null : path.equals(that.path);  

is

return Objects.equals(line, that.line) && Objects.equals(path, that.path);

[ivangalkin]

@guwirth you've done huge effort converting all for-loops into streams with lambdas. What was the reason for that? In my opinion, the code become more complex, less readable. The new code doesn't use any of stream benefits (declarative/functional approach with maps, filters, collects, distincts, not to mention parallelism). If we want such conversion to be done for the loops, I would prefer more careful approach: rewrite only those loops, where comprehensive stream() operation gains some [readability] benefits. Remaining for-loops are perfectly ok to stay just for-loops IMHO .

[guwirth]

@ivangalkin thanks for your feedback. In general: I always try to keep the changes as small as possible and try to follow a pattern.

• most of your proposals can be done in a next PR
• SonarCloud issues: don't have SQ support in NetBeans, so it's try an error.
• will fix isDebugEnabled
• For my understanding stream.forEach is new and more generic way in Java 8. Readability is a matter of taste. Think after reading this several times it's as easy to understand as for-loops?

[ivangalkin]

@guwirth w.r.t. to the streams: they have their own benefits and disadvantages. Some more details:

https://blog.jooq.org/2015/12/08/3-reasons-why-you-shouldnt-replace-your-for-loops-by-stream-foreach/
https://stackoverflow.com/questions/44180101/in-java-what-are-the-advantages-of-streams-over-loops

In my opinion your change introduces all the disadvantages of streams without using any possible advantages.

[guwirth]

@ivangalkin I integrated most of your hints.

• removed stream()... usage
• removed most isDebugEnabled
• For more detailed samples I had no time. May be you can do it in a PR?