Way to optimize this?

[laredotornado]

Hi,

I'm using Java 1.5. My code checker (PMD plug-in, part of Eclipse
Galileo), is complaining about the line "Integer fileCount = null;" in
the following ...

public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
final String
requestFileDigest) {
Integer fileCount = null;
final Session session = sessionFactory.openSession();
try {
fileCount = (Integer)
session. createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
return fileCount;
}

with the complaint, "Found 'DD' -- anomaly for 'fileCount', lines
123-126". Evidently, it doesn't like the fact that the value of the
variable "fileCount" changes. But I don't know how to rewrite this
code block to satisfy the code checker. Do you?

Thanks, - Dave

 

[Lew]

Contrary to your subject line, this is not an optimization issue.

I'm using Java 1.5.  My code checker (PMD plug-in, part of Eclipse
Galileo), is complaining about the line "Integer fileCount = null;" in
the following ...

  public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
                                                 final String

Please, please, please don't indent so heavily.

requestFileDigest) {
    Integer fileCount = null;

Since you never use this 'null' value it's superfuous and you
shouldn't assign it..

    final Session session = sessionFactory.openSession();

Aren't there exceptions you need to catch here?

    try {
      fileCount = (Integer)

Why is this cast necessary? What is the return type of 'uniqueResult
()'?

        session.        createCriteria(AddressRequestFile..class)

Weird spacing.

        .add(Restrictions.eq("requestFileDigest", requestFileDigest))
        .add(Restrictions.eq("vendor", vendor))
        .add(Restrictions.gt("processingResult", 0))
        .add(Restrictions.isNotNull("processingDate"))
        .setProjection(Projections.rowCount()).uniqueResult();
    } finally {
      session.close();
    }
    return fileCount;
  }

with the complaint, "Found 'DD' -- anomaly for 'fileCount', lines
123-126".  Evidently, it doesn't like the fact that the value of the
variable "fileCount" changes.  But I don't know how to rewrite this
code block to satisfy the code checker.  Do you?

That "code checker" sure gives you obscure, uninformative messages,
doesn't it?

Switch to FindBugs.

I don't know how you read that it doesn't "like" the change to the
'fileCount' variable from that message. Since you don't even indicate
what the line numbers are, it's impossible for us to know anything
about that message. What are lines 123-126, hm?

Incomplete questions can lead to incomplete answers.

I'm going to ignore your "code checker" and just talk about good
sense. It is perfectly fine to reassign values to non-final
variables.

You should minimize the scope of variables, however. You can declare
'fileCount' inside the 'try' block, or eliminate it altogether.

 

[Noel]

Hi,

I'm using Java 1.5.  My code checker (PMD plug-in, part of Eclipse
Galileo), is complaining about the line "Integer fileCount = null;" in
the following ...

  public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
                                                 final String
requestFileDigest) {
    Integer fileCount = null;
    final Session session = sessionFactory.openSession();
    try {
      fileCount = (Integer)
        session.        createCriteria(AddressRequestFile..class)
        .add(Restrictions.eq("requestFileDigest", requestFileDigest))
        .add(Restrictions.eq("vendor", vendor))
        .add(Restrictions.gt("processingResult", 0))
        .add(Restrictions.isNotNull("processingDate"))
        .setProjection(Projections.rowCount()).uniqueResult();
    } finally {
      session.close();
    }
    return fileCount;
  }

Integer fileCount = null;
try {
fileCount = ...;
} catch (...) {
fileCount = null;
} finally {
...
}
return fileCount;

Of course, it is advisable to minimize the scope of fileCount, so one
could instead do this (or a variation):

try {
Integer fileCount = ...;
return fileCount;
} catch (...) {
return null;
} finally {
...
}

But then your code checker may complain about having more than one
return statement in a method.

with the complaint, "Found 'DD' -- anomaly for 'fileCount', lines
123-126".  Evidently, it doesn't like the fact that the value of the
variable "fileCount" changes.  But I don't know how to rewrite this
code block to satisfy the code checker.  Do you?

It's unfortunate than an application intended to improve clarity of
code emits unhelpful messages. I'm unfamiliar with this code checker,
but a cursory Google search turned up that this particular rule
belongs to its "Controversial" rule set (http://pmd.sourceforge.net/
rules/controversial.html).

 

[laredotornado]

    Integer fileCount = null;
    try {
        fileCount = ...;
    } catch (...) {
        fileCount = null;
    } finally {
       ...
    }
    return fileCount;

Of course, it is advisable to minimize the scope of fileCount, so one
could instead do this (or a variation):

    try {
        Integer fileCount = ...;
        return fileCount;
    } catch (...) {
        return null;
    } finally {
       ...
    }

But then your code checker may complain about having more than one
return statement in a method.


It's unfortunate than an application intended to improve clarity of
code emits unhelpful messages.  I'm unfamiliar with this code checker,
but a cursory Google search turned up that this particular rule
belongs to its "Controversial" rule set (http://pmd.sourceforge.net/
rules/controversial.html).

PMD is mandated by our company so sadly I don't have control over the
decision to use it at this time. Noel, per your suggestion, the code
checker does complain about the return statement needing to be the
last line in the method when I switch to what you have, although I do
like how you've reduced the scope of the variable.

- Dave

 

[Noel]

Noel, per your suggestion, the code
checker does complain about the return statement needing to be the
last line in the method when I switch to what you have, although I do
like how you've reduced the scope of the variable.

To satisfy both rules, it seems to me that the scope of fileCount
cannot be minimal.

Integer fileCount; // don't initialize
try {
fileCount = ...;
} catch (...) {
fileCount = null;
} finally {
...
}
return fileCount;

I suspect a NullAssignment complaint will be raised. (Good grief.)

 

[Noel]

Noel, per your suggestion, the code
checker does complain about the return statement needing to be the
last line in the method when I switch to what you have, although I do
like how you've reduced the scope of the variable.

To satisfy both rules, it seems to me that the scope of fileCount
cannot be minimal.

Integer fileCount; // don't initialize
try {
fileCount = ...;
} catch (...) {
fileCount = null;
} finally {
...
}
return fileCount;

I suspect a NullAssignment complaint will be raised. (Good grief.)

 

[Roedy Green]

public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
final String
requestFileDigest) {
Integer fileCount = null;
final Session session = sessionFactory.openSession();
try {
fileCount = (Integer)
session. createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
return fileCount;
}

I have stared and stared at the code. I don't see anything wrong with
it. fileCount is always assigned a value. You didn't mark it final.
Maybe this lint dislikes variables being redefined, wanting everything
final.

--
Roedy Green Canadian Mind Products
http://mindprod.com

Without deviation from the norm, progress is not possible.
~ Frank Zappa (born: 1940-12-21 died: 1993-12-04 at age: 52)

 

[Lew]

Noel, per your suggestion, the code
checker does complain about the return statement needing to be the
last line in the method ...

That's not a valid rule.

 

[Noel]

I have stared and stared at the code. I don't see anything wrong with
it. fileCount is always assigned a value. You didn't mark it final.
Maybe this lint dislikes variables being redefined, wanting everything
final.

The PMD online documentation reads: "DD - Anomaly: A recently defined
variable is redefined. This is ominous but don't have to be a bug."
Very picky lint.

 

[Eric Sosman]

The PMD online documentation reads: "DD - Anomaly: A recently defined
variable is redefined. This is ominous but don't have to be a bug."
Very picky lint.

Does anybody understand what they mean by "defined" and
"redefined?" The only thing that happens to fileCount in the
original code is that it's declared with an initial value, then
assigned to, and finally used as the source of the method's
returned value. Where's the "redefinition?"

PMD also mentions a rule that objects when "a recently
defined variable is undefined." What in the world do they
mean by that? The only way I know to "undefine" a variable
is to let it go out of scope.

 

[Eric Sosman]

Don't know this particular code checker but IMHO it is right complaining and
this would make it happier (that's what Lew suggested actually):

final Session session = sessionFactory.openSession();
try {
return
session.createCriteria....
}
finally {
session.close();
}

return null;

.... or the method "falls off the end" and won't compile. And
then you've got two `return' statements, which will probably
earn still another scolding from this disagreeable tool.

 

[Daniel Pitts]

Hi,

I'm using Java 1.5. My code checker (PMD plug-in, part of Eclipse
Galileo), is complaining about the line "Integer fileCount = null;" in
the following ...

public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
final String
requestFileDigest) {
Integer fileCount = null;
final Session session = sessionFactory.openSession();
try {
fileCount = (Integer)
session. createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
return fileCount;
}

with the complaint, "Found 'DD' -- anomaly for 'fileCount', lines
123-126". Evidently, it doesn't like the fact that the value of the
variable "fileCount" changes. But I don't know how to rewrite this
code block to satisfy the code checker. Do you?

Thanks, - Dave

public Integer
getVendorRequestFileDigestCount(final YouthfulDriverVendor vendor,
final String requestFileDigest) {
final Integer fileCount; // make final and don't assign yet.
final Session session = sessionFactory.openSession();
try {
fileCount = (Integer)
session.createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
return fileCount;
}

That is the easiest way. Note, you can simplify it even more:

public Integer
getVendorRequestFileDigestCount(final YouthfulDriverVendor vendor,
final String requestFileDigest) {
final Session session = sessionFactory.openSession();
try {
return (Integer)
session.createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
}

no fileCoount variable worry about anywhere.

 

[Daniel Pitts]

return null;

.... or the method "falls off the end" and won't compile. And
then you've got two `return' statements, which will probably
earn still another scolding from this disagreeable tool.

Adding that return null is a compile error.

 

[Jim Janney]

Hi,

I'm using Java 1.5. My code checker (PMD plug-in, part of Eclipse
Galileo), is complaining about the line "Integer fileCount = null;" in
the following ...

public Integer getVendorRequestFileDigestCount(final
YouthfulDriverVendor vendor,
final String
requestFileDigest) {
Integer fileCount = null;
final Session session = sessionFactory.openSession();
try {
fileCount = (Integer)
session. createCriteria(AddressRequestFile.class)
.add(Restrictions.eq("requestFileDigest", requestFileDigest))
.add(Restrictions.eq("vendor", vendor))
.add(Restrictions.gt("processingResult", 0))
.add(Restrictions.isNotNull("processingDate"))
.setProjection(Projections.rowCount()).uniqueResult();
} finally {
session.close();
}
return fileCount;
}

with the complaint, "Found 'DD' -- anomaly for 'fileCount', lines
123-126". Evidently, it doesn't like the fact that the value of the
variable "fileCount" changes. But I don't know how to rewrite this
code block to satisfy the code checker. Do you?

Thanks, - Dave

The complaint is simply that you're initializing a variable to a value
that is never used, just as if you had written

int i = 4;
i = 15;

The minimal fix is to remove the useless initialization for fileCount: replace
Integer fileCount = null;
with
Integer fileCount;

But I think Michal Kleczek's and Daniel Pitts' solution is better
overall. Putting the return statement after the finally clause
creates the impression that the return value might be calculated in
more than one way.

 

[Eric Sosman]

[...]
final Session session = sessionFactory.openSession();
try {
return
session.createCriteria....
}
finally {
session.close();
}

return null;

.... or the method "falls off the end" and won't compile. And
then you've got two `return' statements, which will probably
earn still another scolding from this disagreeable tool.

Adding that return null is a compile error.

Right you are. Sorry, Michal. <FX: Self-administered
dope slap>

 

[Eric Sosman]

The complaint is simply that you're initializing a variable to a value
that is never used, just as if you had written

int i = 4;
i = 15;

No, the initial value *is* used, if anything in that
chain of method calls throws an exception.

The minimal fix is to remove the useless initialization for fileCount: replace
Integer fileCount = null;
with
Integer fileCount;

I think that would produce a compile error. (But I've
already embarrassed myself once on a similar point in this
thread, so don't take my word for it ...)

 

[Lew]

No, the initial value *is* used, if anything in that
chain of method calls throws an exception.

Since there's no exception handler, an exception would cause the execution to
leave the method and the initial value would not be used.

Since the method as shown has no checked exceptions, only a runtime exception
can happen.

I think that would produce a compile error. (But I've

Nope. Shouldn't. I use a similar idiom quite frequently.

For example, to initialize a connection:

Connection cxn;
try
{
cxn = obtainConnection();
}
catch ( SomeException exc )
{
log( exc );
return;
}
assert cxn != null;
// etc.

 

[Jim Janney]

No, the initial value *is* used, if anything in that
chain of method calls throws an exception.

No, it isn't. There's no catch in the try-finally block, so if an
exception is thrown the return statement is never reached and there's
no need for fileCount to have a value.

I think that would produce a compile error. (But I've
already embarrassed myself once on a similar point in this
thread, so don't take my word for it ...)

See above. I notice some other comments in this thread added a catch
block to return null when an exception is thrown, but that's not how
the original code works (and in this case I think returning null would
be the wrong thing to do).

 

[Eric Sosman]

[...]
I think that would produce a compile error. (But I've
already embarrassed myself once on a similar point in this
thread, so don't take my word for it ...)

s/once/twice/

(Sigh.)

 

[Lew]

Well, you're not going to get away from the _assignment_ in Java,
it's just a question of whether it's done implicitly (Object value
or explicitly (Object value = null

Wrong. The implicit assignment doesn't exist for local variables. The OP's
example

Integer fileCount = null;

for a local variable would not have assigned 'null' if the declaration had been

Integer fileCount;

As a matter of style, I prefer to write them out explicitly to separate
"this variable is supposed to start as null" from "the programmer forgot
to initialize this variable."

As a matter of avoidance of compilation error, a local variable must be
definitely assigned prior to use. If "the programmer forgot to initialize
this variable" the program would not compile when the variable was read.

Furthermore, the superfluous assignment of 'null' hides a failure to assign
the desired value, which would otherwise be a compiler error rather than a
runtime exception. As a matter of good engineering, which trumps "style", it
is better to catch mistakes at compilation than run time.

For primitive types where an initial assignment at declaration can be
superfluous, I still prefer to provide an initial value (if possible,
something out of the range of what the variable will be assigned.) This
is less a matter of style and more a matter of making bugs behave
consistently. (With modern compilers and code analysers, this isn't
really needed in Java -- the use of a variable before it's assigned to
should be detected during coding -- but it's an old habit of mine and I
still sometimes program in other languages so I don't see any reason to
change it.)

In Java if you read a local variable before it's assigned the compiler barfs.
In some cases, it's even harmful to assign a superfluous initial value.

Integer fileCount = null;
try
{
Integer fileCount = expression();
}
finally
{
closeResources();
}
return fileCount;

I object to the superfluous assignment of zeroesque initial values to member
variables also, since any Java programmer already knows it happens on
instantiation. The overhead of the repeated 'null' assignment (or zero or
'false') is small, but why incur it at all if it isn't needed?