What's wrong with my Class?

[Funan]

After written this simple class, I was told it is way too flawed. May
it be my improper use of ArrayList? Please give your suggestions,
thanks in advance.

package filesearch;

import java.util.ArrayList;
import java.util.List;
import java.io.File;

public class FileNameSearch implements FileSearch
{
public List<String> searchFor(String fileName, String path)
{
List<File> files = getDirectoryContent(path);
List<String> matches = new ArrayList<String>();
for (File file : files)
{
if ((file.getName()).equals(fileName))
{
matches.add(file.getAbsolutePath());
}
}
return matches;
}

public List<File> getDirectoryContent(String path)
{
File directory = getDirectory(path);
if (!directory.exists())
{
return new ArrayList<File>();
}
return getFiles(directory);
}

public List<File> getFiles(File directory)
{
File[] files = getFilesInDirectory(directory);
ArrayList<File> result = new ArrayList<File>();
for (File file : files)
{
if (file.isFile())
{
result.add(file);
}
else
{
result.addAll(getFiles(file));
}
}
return result;
}

protected File getDirectory(String path)
{
return new File(path);
}

protected File[] getFilesInDirectory(File directory)
{
return directory.listFiles();
}

 

[Roedy Green]

implements FileSearch

where's the code for that interface?

The easiest way to for someone to evaluate your code is compile and
run it. You need the complete program for that.

 

[Niels Dybdahl]

After written this simple class, I was told it is way too flawed. May

it be my improper use of ArrayList? Please give your suggestions,
thanks in advance.

It does not look very flawed to me. I have not tried to run it, so there
might be some errors, but otherwise it looks ok.
However it uses a lot of RAM, because you create a complete list of all
files before you search the list. On some servers folders, you might even
run out of heap space.
You would use much less RAM and get a much better performance, if you would
use listFiles with a FilenameFilter instead.

Niels Dybdahl

 

[Thomas Hawtin]

After written this simple class, I was told it is way too flawed. May
it be my improper use of ArrayList? Please give your suggestions,
thanks in advance.

I'm curious as to why you didn't ask the person who told you it is way
too flawed.

Tom Hawtin

 

[zero]

After written this simple class, I was told it is way too flawed. May
it be my improper use of ArrayList? Please give your suggestions,
thanks in advance.

<code snipped>


Who told you it was way too flawed? That really depends on what you want
to do with it. In addition to what other people already said, let me give
you a few hints.

1. You may be taking structural programming a bit too far. The
getDirectory(String path) method adds method call overhead for something as
simple as

new File(path)

It only seems to be used in getDirectoryContent. Why not delete that
method and replace the method call with

File directory = new File(path);

Yes it's bad programming practice to have methods that are too long, but
the opposite just adds overhead for no reason.

2. Same goes for getFilesInDirectory(File directory)

2b. Some optimizing compilers may inline simple methods for you, but it's
better to write good code than to rely on the compiler.

3. Your class does not depend on any data members, so why not make the
methods static? That way they can be called without having to instantiate
the class. This saves system resources.

zero

 

[Chris Uppal]

3. Your class does not depend on any data members, so why not make the
methods static? That way they can be called without having to instantiate
the class. This saves system resources.

Poor advice I feel. Much better to stay within the OO framework (unless, of
course, you choose to reject OO altogether -- in which case you should also
reject Java). For instance, the code could be made simpler by maintaining a
List of matching files as part of the instance state. Recurse to scan the
directory tree and add matching files to that as they are found. Similarly the
filename pattern could be part of the instance state. You might not even
/think/ of such options if you restrict yourself to static methods and a non-OO
mindet. (And even if you did think of it, you couldn't take the option without
changing the external interface.)

-- chris

 

[zero]

Poor advice I feel. Much better to stay within the OO framework
(unless, of course, you choose to reject OO altogether -- in which
case you should also reject Java). For instance, the code could be
made simpler by maintaining a List of matching files as part of the
instance state. Recurse to scan the directory tree and add matching
files to that as they are found. Similarly the filename pattern could
be part of the instance state. You might not even /think/ of such
options if you restrict yourself to static methods and a non-OO
mindet. (And even if you did think of it, you couldn't take the
option without changing the external interface.)

-- chris

Well I was just commenting on the class as it was, which didn't use any
data members. In general, static methods require only one copy in memory,
so it's more cost effective. But, in this case the code was creating Lists
within the methods, and re-doing that every time it was needed would again
increase the cost.

From a pure OO standpoint you may be right, but on the other hand static
members and methods are available, so why not use them?

Like I said in my first sentence in reply to the OP, it all depends on what
you need. If you never need to search the same directories or search for
the same files, static might be better (although with the code as it is,
I'm not really convinced of that, I'm talking about general concepts here).
If you have to search the same trees over and over, keeping them in memory
is obviously better.

 

[Oliver Wong]

In general, static methods require only one copy in memory,
so it's more cost effective.

I believe that there's only ever one copy of methods in memory anyway,
whether they are static or not. That is, even if a method is non-static, the
code for that method does not change from one instance of an given class
from the next, so you only need to store that method in memory once, and
every instance can have a reference to that same memory location.

The nice thing about static methods is that you don't need an instance
of a class to invoke its static methods.

- Oliver

 

[Chris Uppal]

In general, static methods require only one copy in memory,
so it's more cost effective.

Static methods are no more expensive than non-static. There is only one copy
of each.

From a pure OO standpoint you may be right, but on the other hand static
members and methods are available, so why not use them?

<shrug/> Why make it hard for yourself ?

I suspect that the criticism of the OP's first effort was (at least in part)
because s/he had just written completely non-OO code and wrapped a "class"
around it. Had s/he not done that then there would have been no reason (not
even a bad reason like not wanting to pass an extra argument around everywhere)
not to filter the list of files as it was created instead of building the whole
list, /then/ filtering.

-- chris