Code analyzers: PMD & FindBugs

Question

1. Regarding PMD:

1.1 How do I set the PMD checks, to ignore some of them, like "Variable name is too short, or too long", "Remove empty constructor, etc" - and if I do that, another warning appears that says the class must have some static methods. Basically, the class was empty, for later development, and I like to leave it that way for now.

1.2 Is it necesarry to follow this warning advice?

  A class which only has private constructors should be final

1.3 What is that supposed to mean?

 The class 'Dog' has a Cyclomatic Complexity of 3 (Highest = 17)

1.4 What about this one? I would love to change this, but nothing crosses my mind at the moment regarding the change:

Assigning an Object to null is a code smell. Consider refactoring.

2.Regarding FindBugs:

2.1 Is it really that bad to write to a static field, at some point later than its declaration? The following code gives me a warning:

Main.appCalendar = Calendar.getInstance();
Main.appCalendar.setTimeInMillis(System.currentTimeMillis());

where appCalendar is a static variable.

2.2 This code:

strLine = objBRdr.readLine().trim();

gives the warning:

Immediate dereference of the result of readLine()

where objBRdr is a BufferedReader(FileReader). What could happen? readLine() could be null? The code is nested in while (objBRdr.ready()) test, and so far, I have zero problems there.

Update1: 2.2 was fixed when I replaced the code with:

strLine = objBRdr.readLine();
    if (strLine != null) {
        strLine = strLine.trim();
    }

Answer 1

Here some idea / answer

1.4 What is the reason to assign null to a object? If you reuse the same variable, there's not reason to set it to null before.

2.1 The reason about this warning, is to be sure that all your instance of the class Main have the same static fields. In your Main class, you could have static Calendar appCalendar = Calendar.getInstance() ;

about your 2.2 you're right, with the null check, you are sure that you'll not have any NullPointerException. We never know when your BufferedReader can block/trash, this doesn't happen often (in my experience) but we never know when a hard drive crash.

Answer 2

1.1 How do i set the PMD checks [...]

PMD stores rule configuration in a special repository referred to as the Ruleset XML file. This configuration file carries information about currently installed rules and their attributes.

These files are located in the rulesets directory of the PMD distribution. When using PMD with Eclipse, check Customizing PMD.

1.2 Is it necessary to follow this warning advice?

A class which only has private constructors should be final

All constructors always begin by calling a superclass constructor. If the constructor explicitly contains a call to a superclass constructor, that constructor is used. Otherwise the no-argument constructor is implied. If the no-argument constructor does not exist or is not visible to the subclass, you get a compile-time error.

So it's actually not possible to derive a subclass from a class whose every constructor is private. Marking such a class as final is thus a good idea (but not necessary) as it explicitly prevent subclassing.

1.3 What is that supposed to mean?

The class 'Dog' has a Cyclomatic Complexity of 3 (Highest = 17)

The complexity is the number of decision points in a method plus one for the method entry. The decision points are 'if', 'while', 'for', and 'case labels'. Generally, 1-4 is low complexity, 5-7 indicates moderate complexity, 8-10 is high complexity, and 11+ is very high complexity.

Having that said, I'll just quote some parts of Aggregate Cyclomatic complexity is meaningless:

[...] This metric only has meaning in the context of a single method. Mentioning that a class has a Cyclomatic complexity of X is essentially useless.

Because Cyclomatic complexity measures pathing in a method, every method has at least a Cyclomatic complexity of 1, right? So, the following getter method has a CCN value of 1:

public Account getAccount(){
   return this.account;
}

It’s clear from this boogie method that account is a property of this class. Now imagine that this class has 15 properties and follows the typical getter/setter paradigm for each property and those are the only methods available. That means the class has 30 simple methods, each with a Cyclomatic complexity value of 1. The aggregate value of the class is then 30.

Does this value have any meaning, man? Of course, watching it over time may yield something interesting; however, on its own, as an aggregate value, it is essentially meaningless. 30 for the class means nothing, 30 for a method means something though.

The next time you find yourself reading a copasetic aggregate Cyclomatic complexity value for a class, make sure you understand how many methods the class contains. If the aggregate Cyclomatic complexity value of a class is 200– it shouldn’t raise any red flags until you know the count of methods. What’s more, if you find that the method count is low yet the Cyclomatic complexity value is high, you will almost always find the complexity localized to a method. Right on!

So to me, this PMD rule should be taken with care (and is actually not very valuable).

1.4 What about this one? I would love to change this, but nothing crosses my mind at the moment regarding the change:

Assigning an Object to null is a code smell. Consider refactoring.

Not sure what you don't get about this one.

2.1 Is it really that bad to write to a static field, at some point later than its declaration? [...]

My guess is that you get a warning because the method contains an unsynchronized lazy initialization of a non-volatile static field. And because the compiler or processor may reorder instructions, threads are not guaranteed to see a completely initialized object, if the method can be called by multiple threads. You can make the field volatile to correct the problem.

2.2 [...] Immediate dereference of the result of readLine()

If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception. So you need indeed to check if the result is null.