Way to optimize this?

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()).uniqueResul t();
} 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

laredotornado

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

laredotornado wrote:
> 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()).uniqueResul t();
> * * } 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.

--
Lew

Lew

On Nov 6, 9:10*am, laredotornado <laredotorn...@zipmail.com> wrote:
> 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()).uniqueResul t();
> * * } 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).

Noel

On Nov 6, 10:48*am, Noel <prosthetic_conscien...@yahoo.com> wrote:
> On Nov 6, 9:10*am, laredotornado <laredotorn...@zipmail.com> wrote:
>
>
>
> > 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()).uniqueResul t();
> > * * } 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).

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

laredotornado

On Nov 6, 10:08*am, laredotornado <laredotorn...@zipmail.com> wrote:
> 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

On Nov 6, 10:08*am, laredotornado <laredotorn...@zipmail.com> wrote:
> 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

On Fri, 6 Nov 2009 09:10:07 -0800 (PST), laredotornado
<> wrote, quoted or indirectly quoted someone
who said :

> 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()).uniqueResul t();
> } 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)

Roedy Green

laredotornado wrote:
> 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.

--
Lew

Lew

On Nov 6, 11:21*am, Roedy Green <see_webs...@mindprod.com.invalid>
wrote:
> 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.

Noel

Noel wrote:
> On Nov 6, 11:21 am, Roedy Green <see_webs...@mindprod.com.invalid>
> wrote:
>> 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.

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
lid

Eric Sosman