views:

191

answers:

9

Here's an example of a utility method:

public static Long getFileSize(String fileString) {

    File file = new File(fileString);

    if (file == null || !file.isFile())
        return null;

    return file.length();
}

Is it a good practise to pass a String rather than a File to a method like this? In general what reasoning should be applied when making utility methods of this style?

A: 

The only thing that matters is how you're gonna use this method. In other words, if your application operates with File objects, you could pass them and remove some unnecessary operations. If you operate with file paths, string parameter may be more convenient.

But ultimately, neither choice is wrong: neither will make your application work worse.

Nikita Rybak
+1  A: 

I would think this would depend upon what you have available at the point where you are going to be calling this method. If you have the file name (String), but no File, there seems little point in making the caller create the File from the file name.

My approach to this when I'm not sure is to create two methods, one for String, one for File. Then have the String one create the file and call the file one.

public static long getFileSize (final String fileString) {
  return getFileSIze (new File (fileString)); 
}

public static long getFileSize (File file) {
   if (file == null || !file.isFile()) return null;
   return file.length();
}
Thomas Jones-Low
+1  A: 

It depends on the utility expected from the client side. In case the client side already has a file object and wants to fetch the filesize, the client side developer is forced to extract the file path and pass it to the utility method. In order to avoid it, I would provide overloaded methods 1) with File 2) File Path String

Also, In case the file is unavailable, I would throw an exception than return null.

Snehal
There is zero advantage in overloading here, use distinct names unless there is some justification. http://stackoverflow.com/questions/248222/method-overloading-can-you-overuse-it
leonbloy
@leonbloy - What would you consider justification? For example, in the question you linked, the accepted answer explicitly recommends overloads for, among other things, filename as string, and the File.
CPerkins
@CPerkins: see my response to binil, and the quote from Joshua Bloch. Method overloading (with same number of parameters) is the root of many evils specially when: the alternative types are one a subclass of another or when used in setters (or in general methods that some framework will lookup in runtime through reflection). I agree that these agravating factors do not apply here, but anyway, I'm conservative about this, and do not agree with that recommendatoin.
leonbloy
@leonbloy Agreed in general, and the examples Dr. Bloch gives are clear. I am slightly less conservative than you in this regard - specifically here, with File and String representations of filename/path, I feel there's such clear parallelism that there's more *surprise* in having them named differently than by overloading.
CPerkins
+1  A: 

My recommendation would be to have both:

public static Long getFileSize(String path) {
    return getFileSize(new File(path));
}

public static Long getFileSize(File file) {
    return (file == null || !file.isFile()) ? 0L : file.length();
}

and let your users choose based on the the kind of object they are using to represent filesystem paths. As @Nikita mentioned, neither choice is wrong.

binil
I'd give you +1 ... if it weren't for the `file == null` test. That case is a design error in the original method ... it should just let an NPE happen.) Also you should return -1L instead of 0L for "not a file" to distinguish the case of a file with zero bytes.
Stephen C
@Stephen Yes, I agree with everything .. except for the +1 part. :-)
binil
Method overloading is frequently overused, and this is a example. There is nothing to gain (except some future pain) by calling the two methods the same in this case. Read here http://stackoverflow.com/questions/248222/method-overloading-can-you-overuse-it
leonbloy
@leonbloy How different is the code above from the DeleteFile example from @Bevan's post (which you seem to 'agree wholeheartedly') in the thread you linked to?
binil
leonbloy
@leonbloy Bloch also says "Exporting multiple overloadings with the same number of parameters is unlikely to confuse programmers if it is always clear which overloading will apply to any given set of actual parameters. This is the case when at least one corresponding formal parameter in each pair of overloadings has a “radically different” type in the two overloadings. Two types are radically different if it is clearly impossible to cast an instance of either type to the other.". I hope we can agree that java.lang.String and java.io.File are "radically different" types.
binil
+1  A: 

In my opinion, that function is only useful with a string parameter. What does it do?

  • It creates a file object.
  • Checks that it can be created.
  • Checks that it's a file
  • Returns the length

If you passed it a file, the first thing isn't needed, the next two should probably be assumed, and the length is a file member function. If you pass this a file, this function becomes too trivial to write :)

(Also, I think returning null from a function that returns a long is strange)

If you have a File object already, use:

length = file.isFile() ? file.length() : -1;

If your code deals with files instead of file names you could save yourself some file opens if you reuse the File pointers. In that case, it might lead you to use them over the filename approach.

Stephen
That code does not open files! It creates a "fancy String wrapper" (the `File` object) and uses that to lookup the file in the file system (in `isFile()` and `length()`). Because the lookup is native, I don't see whether one might save time or resources by reusing the `File` object.
Christian Semrau
Stephen
+2  A: 

This is my preferred solution:

public static Long getFileSize(String path) {
    return getFileSize(new File(path));
}

public static Long getFileSize(File file) {
    return (!file.isFile()) ? -1L : file.length();
}

Note that it is returning -1L rather than 0L, to allow the caller to distinguish between an empty file, and a "file" whose length cannot be determined for some reason. The file.length() will return zero in some cases where you don't have a zero length file; e.g.

  • when the file does not exist
  • when the file is a directory
  • when the file is a special file (e.g. device file, pipe, etc) and the OS cannot determine its length.

The file.isFile() call deals with these cases. However, it is debatable whether the method(s) should return -1L or throw an exception. The answer to this debate turns on whether the -1L cases are "normal" or "exceptional", and that can only be determined with reference to the contexts in which the method is designed to be used,

Stephen C
+1  A: 

There are a number of considerations:

  1. Utility methods exist to reduce the amount of repetitive boiler plate code in your app, hence making the code more readable and reducing the number of potential bugs. It makes sense to cater for most common usage patterns, i.e. if most of the time you have a string describing a file - pass the string. Most of the benefit comes from having a utility method in a first place, not getting the optimal signature that is 100% future-proof.

  2. Passing a file instead of a string provides stronger typing, that is to say more of you code can be checked at compile time safeguarding against typos. Make the compiler do the work for you, use the benefits of strong typing.

  3. Passing a file means that you could pass any kind of File object, possibly a bespoke in-memory file object without having to change the utility method to handle the bespoke file descriptor.

  4. Passing a string could help when you have to deal a lot with OS file paths and you just want to check the size with a minimum number of lines.

  5. At the end you could have several overloaded utility methods at a very low cost. This scenario is exactly the reason for existence of method overloading as a language feature. See what naturally works in you code base. Code is malleable and this is not one of these design decisions you'd have to live with forever, unless you're building an API for other people to use.

  6. You could also want to change the name to be a bit more descriptive, for instance

    • long sizeFromFile(File f) and
    • long sizeFromFileName (String name)

    using convention originally suggested by Joel Spolsky.

Totophil
+1  A: 

I'd go with a File. It feels a little OOP correct to me: more typesafe (Strings are so 'general' in Java...) and semantically expressive: if you are dealing with files, well then pass a File object.

Recall that in Java a File object represents not really a file in itself (its content) but rather its path: "An abstract representation of file and directory pathnames" (it can even be the path of a non-existent file) and that's almost exactly what you need here.

This can only be a limitation in a few cases: if the "file" is actually some kind of pseudo-file or resource, for example inside a jar file. An alternative I have found useful is a URI, which (conceptually) includes a File as a special case, but also other resources.

And if you decide to stick with the two alternatives (String or File), I emphatically don't recommend to name the methods the same. Method overloading can be a pain, use it only when it gives you a tangible benefit.

leonbloy
+1  A: 

Method Overloading is the best practice in such cases.

Phani