2
\$\begingroup\$

Let me know if the given code can be improved or optimized:

public static String getFileNameWithoutExtensiotn(File file) {
    String fileName = null;
    if (file.getName().startsWith(".")) {//if starts with "." like .htaccess
        fileName = file.getName();
    } else {
        try {
            fileName = file.getName().substring(0, file.getName().lastIndexOf("."));
        } catch (Exception e) {
            fileName = file.getName();
        }
    }

    return fileName;
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ First improvement: remove spelling mistake in method name. Second thought: What about .htaccess.txt ? \$\endgroup\$
    – Teetrinker
    Jul 23, 2015 at 13:17
  • \$\begingroup\$ @Teetrinker Do not answer questions in the comments section. Improvements are meant for their own answers. \$\endgroup\$
    – SirPython
    Jul 23, 2015 at 13:18
  • 1
    \$\begingroup\$ @SirPython Sorry, it did not feel thorough enough for an answer. \$\endgroup\$
    – Teetrinker
    Jul 23, 2015 at 13:21
  • 1
    \$\begingroup\$ @Teetrinker Read this and this. In conclusion, short answers are perfectly okay as long as you add sufficient context to them. \$\endgroup\$
    – SirPython
    Jul 23, 2015 at 13:23

4 Answers 4

6
\$\begingroup\$

Your code can be improved in few lines:

        String fileName = file.getName();
        if (fileName.indexOf(".") > 0)
            fileName = fileName.substring(0, fileName.lastIndexOf("."));
        return fileName;

As far as catching the exception, an advice, always catch the specific exception, not the generic, so you should be catching IOException or IndexOutOfBoundException instead of Exception. Also, it's always a good idea to log exception somehow.

\$\endgroup\$
3
\$\begingroup\$

Note that you misspelled your function name - there's no t in there.

Your use of lastIndexOf is great, but there's no need to combine it with startsWith too.

Additionally, you should get familiar with early-return concepts. They help make code more readable. You also get the file's name multiple times, which is a waste of effort. Consider the much simpler version of your function:

public static String getFileNameWithoutExtension(File file) {
    String name = file.getName();
    int pos = name.lastIndexOf('.');
    if (pos > 0 && pos < (name.length() - 1)) {
        // there is a '.' and it's not the first, or last character.
        return name.substring(0,  pos);
    }
    return name;
}

If you are familiar with regular expressions they can be helpful in cases like this too. A file extension is anything after a . that's not a . and is not at the string's beginning.

So, this requires a pattern that uses a positive lookbehind, and then a generic match on the extension:

private static final Pattern ext = Pattern.compile("(?<=.)\\.[^.]+$");

public static String getFileNameWithoutExtension(File file) {
    return ext.matcher(file.getName()).replaceAll("");
}

Just a thought, probably neither more efficient, nor more readable for a non-familiar person, but you do see code like that, and it's helpful to know your alternatives.

\$\endgroup\$
2
\$\begingroup\$

Here you are catching an exception:

} catch (Exception e) {
    fileName = file.getName();
}

While you did a good job knowing to catch an exception, you are being very general; you are just capturing Exception and tells the person who is reading your code absolutely nothing.

Rather than just catching the generic Exception, you should be more specific and catch the exception that will be coming out of that line.

I think the exception coming out of this would be from passing -1 to String.substring, which would happen in there was no last index of ".".

After reading the documentation, I discovered that the exception that would be raised from this line is the IndexOutOfBoundsException.

Now, rather than catching the generic Exception, you should catch IndexOutOfBoundsException.


A simpler, maybe faster way of getting the string before the "." would be to just split the String by the ".". Then, you'd be left with an array containing the String before the ".".

Now, getting the name would be this:

fileName = file.getName().split("\\.", 2)[0];

Note: I passed a 2 as a second parameter to split because this limits how many times the string will be split up. By passing a 2, this limits to the string only being split a single time, which might speed up performance.

Note two: since split is looking for a regexular expression and . is a valid token, I inserted an escape character.

\$\endgroup\$
3
  • \$\begingroup\$ thanks @SirPython for the review ... by the way i just decide not show whats is going on in Catch so i did so,,,,, \$\endgroup\$
    – mak
    Jul 23, 2015 at 13:46
  • 1
    \$\begingroup\$ If you have a.b.txt this will return... \$\endgroup\$
    – h.j.k.
    Jul 23, 2015 at 14:31
  • \$\begingroup\$ If you have a.b.txt, I think this will return a. \$\endgroup\$
    – SirPython
    Jul 23, 2015 at 15:04
2
\$\begingroup\$

You can so this to avoid scanning the string more than once.

String fileName = file.getName();
int last = fileName.lastIndexOf(".");
return last >= 1 ? fileName.substring(0, last) : fileName;

This changes the behaviour slightly in that .hidden.txt will return .hidden

\$\endgroup\$
1
  • \$\begingroup\$ The shortest and the cleanest way. \$\endgroup\$
    – Sasha
    May 12, 2019 at 7:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.