Way to optimize this?

Michal Kleczek wrote:
> laredotornado 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;
>> }
>>
>> 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?
>>
>
> 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.

--
Eric Sosman
lid

Eric Sosman

laredotornado 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;
> }
>
> 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()).uniqueResul t();
} 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()).uniqueResul t();
} finally {
session.close();
}
}

no fileCoount variable worry about anywhere.

Daniel Pitts

Eric Sosman wrote:
> Michal Kleczek wrote:
>> laredotornado 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;
>>> }
>>>
>>> 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?
>>>
>>
>> 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.

Adding that return null is a compile error.

Daniel Pitts

laredotornado <> writes:

> 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

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.

--
Jim Janney

Jim Janney

Daniel Pitts wrote:
> Eric Sosman wrote:
>> Michal Kleczek wrote:
>>> [...]
>>> 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
lid

Eric Sosman

Jim Janney wrote:
> laredotornado <> writes:
>
>> 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
>
> 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 ...)

--
Eric Sosman
lid

Eric Sosman

laredotornado writes:
>>> 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;
>>> }

Jim Janney wrote:
>> 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;

Eric Sosman wrote:
> 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.

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

Eric Sosman wrote:
> 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.

--
Lew

Lew

Eric Sosman <> writes:

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

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.

>> 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 ...)

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).

--
Jim Janney

Jim Janney

Eric Sosman wrote:
> [...]
> 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.)

--
Eric Sosman
lid

Eric Sosman

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

Leif Roar Moldskred wrote:
> 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?

--
Lew

Lew