aspose file tools*
The moose likes Beginning Java and the fly likes This is ugly, but would it cause a leak (or other problem(s))? Big Moose Saloon
  Search | Java FAQ | Recent Topics | Flagged Topics | Hot Topics | Zero Replies
Register / Login


Win a copy of Soft Skills this week in the Jobs Discussion forum!
JavaRanch » Java Forums » Java » Beginning Java
Bookmark "This is ugly, but would it cause a leak (or other problem(s))?" Watch "This is ugly, but would it cause a leak (or other problem(s))?" New topic
Author

This is ugly, but would it cause a leak (or other problem(s))?

Jim Lum
Greenhorn

Joined: Apr 18, 2005
Posts: 6
Hi,

Per the subject, I know that this code/design is really ugly. It's so bad that it just "hurts my head" to stare at it.

BUT... it's code that I'm trying to review, and trying to debug, and hopefully fix...

We have a Java class, say ClassA, that gets instantiated once. It's actually a kind of plugin for a commercial server-type product, to extend it, and our class gets instantiated once when that product is started.

This class, ClassA, in turn, instantiates another class, ClassB, i.e., in the constructor of ClassA, we have:

ClassB foo = new ClassB(xxx);

In ClassB, there's a global (global to ClassB) String variable, theFileName, that gets set in the ClassB's constructor:

String theFileName;
.
.
theFileName = "xxx" + "yyy";

There's a 3rd class, ReadParameterFile, that is used by methods in ClassB to read parameters from a parameter file. This ReadParameterFile class has a global (global to the ReadParameterFile class) String variable, e.g., "paramFileName", and a global (again, global to the ReadparameterFile class) BufferedReader variable, "in", and the following methods:

METHOD - OpenParamFile(String fName): Sets paramFileName from fName, then creates new BufferReader, setting it to "in".



METHOD - CloseParamFile(): Closes "in", then createw new BufferedReader, setting it to "in".




METHOD - GetParam(String param): Reads through the parameter file, trying to find the parameter.




In ClassB, there are a bunch of methods that get called from ClassA. Each of these methods typically start with:




So, here's the problem that I'm trying to chase down:

We have logging all over the place, and in particular, in the OpenParameterFile() method, when it encounters an exception, we log the value of the "paramFileName" variable.

We've noted that when all of the above is started, there are no problems/exceptions, but, over time, we start seeing errors from the OpenParameterFile() method, FileNotFoundException, and when it outputs the value of "paramFileName", it's showing as an empty String (nothing).

As I said above, I KNOW that this code is really ugly (=bad), but it's what I was given, and I'm trying to figure out why that "paramFileName" would start showing nothing, especially since the string value for the file path/name is set in ClassB only once in the ClassB constructor.

Although the code IS ugly, it seems like it should work "ok", i.e., ClassB is instantiating a new ReadParameterFile object each time (and assigning the new BufferedReader object/insance to pFile), and passing that hard-code string file path/name to the ReadParameterFile's OpenParamFile() method each time it does that.

When the pFile variable is re-assigned with:



doesn't that essentially de-reference the pFile variable, and then re-reference to the new ReadParameterFile object instance? And shouldn't the JVM then automatically garbage collect that no longer referenced ReadParameterFile object/instance?

Sorry for the long post. I've been chasing this down for awhile, and as I said, this is literally hurting my head !!

Thanks for the patience.

Jim
Jesper de Jong
Java Cowboy
Saloon Keeper

Joined: Aug 16, 2005
Posts: 14428
    
  23

Here are some things that look strange to me at first glance:

Jim Lum wrote:METHOD - CloseParamFile(): Closes "in", then createw new BufferedReader, setting it to "in".


So, CloseParamFile() closes the input stream and then immediately opens it again? Not what I'd expect from a method that's supposed to close the file.

Jim Lum wrote:

So, a new ReadParameterFile is created for no reason and assigned to pFile. It's unnecessary because pFile is immediately after that assigned to another value. You'd better write:


Also, the naming of the methods is non-standard. Almost all Java developers use camel case for method names, starting with a lower-case letter. So you'd normally call the methods: openParamFile, closeParamFile instead of Open... and Close...

Is there a reason why the file name is stored in a global variable?

Java Beginners FAQ - JavaRanch SCJP FAQ - The Java Tutorial - Java SE 8 API documentation
Jim Lum
Greenhorn

Joined: Apr 18, 2005
Posts: 6
Jesper,

I mistyped that one code snippet where the OpenParameter file is called, it should have been:



i.e., the ReadParameterFile object is instantiated, then the OpenParamFile method in that object/instance is called with the filename.

As far as your last question as to why the file name is stored in a global variable: I'm don't know, as the person who wrote this code is long gone from our project. I'm guessing that it was for "convenience", depending on which global variable/file name you're asking about.

In any event, is there anything in the structure of what I've described that might cause what I described (that file name getting either emptied or cleared)?

Thanks, and sorry for the mis-typing.

Jim
 
I agree. Here's the link: http://aspose.com/file-tools
 
subject: This is ugly, but would it cause a leak (or other problem(s))?