11

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?

James P.
  • 19,313
  • 27
  • 97
  • 155
  • 3
    If they are making a `File` first, I don't think they would bother doing `getFileSize(file)` when they could just do `file.length()`. Also, `file` can't actually be `null` in that situation; constructors don't have the ability to return a null object, it would have to be a factory method like `File makeFile(String filename)` – Michael Mrozek May 09 '10 at 00:43

9 Answers9

3

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
  • 698,415
  • 94
  • 811
  • 1,216
2

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.

Community
  • 1
  • 1
leonbloy
  • 73,180
  • 20
  • 142
  • 190
1

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
  • 7,001
  • 2
  • 32
  • 36
1

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
  • 7,266
  • 2
  • 32
  • 42
  • 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 May 10 '10 at 20:05
  • @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 May 10 '10 at 21:15
  • @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 May 11 '10 at 14:32
  • @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 May 11 '10 at 17:21
1

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 Thomas
  • 13,699
  • 10
  • 57
  • 70
  • 2
    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 May 09 '10 at 03:09
  • 1
    @Stephen Yes, I agree with everything .. except for the +1 part. :-) – Binil Thomas May 09 '10 at 07:31
  • 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 May 10 '10 at 20:01
  • @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 Thomas May 10 '10 at 22:12
  • @binil: The link was to call attention to some discussion in SO about the issue. I'm more severe that Bevan in this regard, I guess. I should better quote the almost biblical "Effective Java" (Joshua Bloch) : "Exactly what constitutes a confusing use of overloading is open to some debate. A safe, conservative policy is never to export two overloadings with the same number of parameters." I adhere (in general, not fanatically) to that policiy. Just from my experience. http://www.informit.com/articles/article.aspx?p=31551&seqNum=4 – leonbloy May 11 '10 at 14:23
  • 1
    @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 Thomas May 18 '10 at 01:12
1

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
  • 47,994
  • 7
  • 61
  • 70
  • 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 May 09 '10 at 06:40
  • @Christian Semrau: I'm not too familiar with java, thanks for pointing that out. Apparently File() doesn't make a system call. But isFile & length both result in system calls (right?). If you're working with file objects elsewhere, it's possible that you already know whether isFile has been called... you probably don't want all your code to check isFile() and handle directories differently. (fixed answer) – Stephen May 09 '10 at 13:42
1

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.

Community
  • 1
  • 1
Vlad Gudim
  • 23,397
  • 16
  • 69
  • 92
1

Method Overloading is the best practice in such cases.

Phani
  • 5,319
  • 6
  • 35
  • 43
0

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
  • 67,365
  • 22
  • 157
  • 181