Dataflow anomaly analysis warnings from PMD

Question

I am using Eclipse with the PMD Plug-in (4.0.0.v20130510-1000) and get a lot of those violations:

Found 'DD'-anomaly for variable 'freq' (lines '187'-'189').
Found 'DU'-anomaly for variable 'freq' (lines '189'-'333').

In this SO answer, it says that those anomalies are related to assigning values that are never read. But I get the violations for instance in this case:

// here I get a DD anomaly
double freq = 0;
try {
  // here I get a DU anomaly
  freq = Double.parseDouble(getFrequencyTextField().getText());
} catch (final NumberFormatException e) {
  Log.e(e.getMessage());
}
if (freq < 10E6) doSomething();

If I remove the initialization and add a freq = 0; line in the catch block, the DD anomaly vanishes, but I get a DU anomaly on both the assignments.

Now my question: How am I supposed to deal with that? What would be the preferred solution of PMD? And what exactly is this rule trying to prevent (i.e. why is it bad practice)?

Joop Eggen · Accepted Answer · 2013-05-27T08:25:37.113

double freq; // (1)
try {
  // here I get a DU anomaly
  freq = Double.parseDouble(getFrequencyTextField().getText());
} catch (final NumberFormatException e) {
  Log.e(e.getMessage());
  freq = 0; // (2)
}
if (freq < 10E6) doSomething();

The first problem is that in the catch the parseDouble assignment is not done to freq. On an exception freq still would be 0. Maybe flaggable. So it goes away when assigning to freq inside catch.

When assigning to freq in the catch, (2), the inital assignment, (1), would never be read, so only a declaration suffices.

With respect to better style:

try {
  // here I get a DU anomaly
  double freq = Double.parseDouble(getFrequencyTextField().getText());

  if (freq < 10E6) doSomething();
  ...

} catch (final NumberFormatException e) {
  Log.e(e.getMessage());
}

Or follow the answer of @JoachimSauer, using a double conversion that does not throw an exception. The logging would indicate a level of seriousness in preference of the above style. Logging inside a simple conversion function on error might not be good style: too much logging, ignored logging (?), hard to repair.

Thanks for the explanation, that cleared up things for me. Also thanks for the suggestion. I always appreciate code style corrections. (That's why I started using PMD in the first place.) — brimborium, May 27 '13 at 08:44

score 4 · Answer 2 · answered May 27 '13 at 08:08

4

You could get around this problem (and separate concerns a bit more clearly) by extracting the parsing into a separate method:

double freq = parseDouble(getFrequencyTextField().getText());

// later in the class (or in a utility class):

public static double parseDouble(final String input) {
  try {
    return Double.parseDouble(input);
  } catch (final NumberFormatException e) {
    Log.e(e.getMessage());
    return 0;
  }
}

And if you have different default values, you could also add a two-argument version: public static double parseDouble(final String input, final double defaultValue).

answered May 27 '13 at 08:08

Joachim Sauer

302,674
57
556
614

I like the default value idea. I think I am going to do that. Although in that example, I just get the next PMD violation: `A method should have only one exit point, and that should be the last statement in the method`. – brimborium May 27 '13 at 08:39
@brimborium: that warning creates an ugly semantic loop: I see no (simple) solution to avoid *both* warnings at the same time. – Joachim Sauer May 27 '13 at 08:40
Yeah, I am inclined to kick that second rule out of the ruleset. Although I understand it's point. It is much easier to understand the full meaning of a complex method if there is only one exit point... – brimborium May 27 '13 at 08:47
1

Personally I don't agree with the one-exit-point rule at all. It often makes the code needlessly complicated. I'd replace it with a "don't-have-exit-points-at-unexpected-places" rule, but that *might* be slightly harder to check for a static analyzer ;-) – Joachim Sauer May 27 '13 at 09:01
1

A return in a catch block is always bad. Add a finally block and you have confusing code. – BevynQ May 30 '13 at 20:35
@BevynQ: I disagree. Sometimes a return in the catch is exactly what you intend to do (such as in this case). A return in a `final` however, is pure evilk. – Joachim Sauer May 31 '13 at 19:54

Dataflow anomaly analysis warnings from PMD

2 Answers2

Linked

Related