Code Smell? - Adjusting variables with +- 1

Question

I just wrote this method:

private String getNameOfFileFrom(String path)
{
 int indexOfLastSeparator = path.lastIndexOf('/');
 if (indexOfLastSeparator > -1)
 {
  return path.substring(indexOfLastSeparator + 1);
 }
 else
 {
  return path;
 }
}

The line that bothers me is:

return path.substring(indexOfLastSeparator + 1);

Is that a bad practice to modify the expression inline like that? Suggestions on how to refactor to increase readability would be most welcome.

----Edit---- OK update after comments. Thanks everyone for answering :) I am not looking to actually change the the value of the variable at all just the expression it is used it.

Another posted suggested I could break out that part of the expression as in the second code snippet below. Better/worse/no difference? :) I am starting to suspect I am being overly cautios here.

return path.substring(indexOfLastSeparator + 1);

or

int indexOfFirstCharInFileName = indexOfLastSeparator + 1;
return path.substring(indexOfFirstCharInFileName);

Answer 1

In this case, you can just do:

private String getNameOfFileFrom(String path)
{
        return path.substring(path.lastIndexOf('/') + 1);
}

At least, in both Java and .NET, x.substring(0) will return an equal string. In Java it will return the same reference - not sure about .NET.

In general though, there are plenty of times where it makes sense to add or subtract one. I don't really think it's a code smell.

Answer 2

If you're using .NET look at System.Io.Path.Combine(string,string) which is using to combine 2 strings without checking and/or removing terminating slashes

Or you can use string.Remove(string.LastIndexOf("/"));

Answer 3

I don't see why that would be a bad practice. Maybe you can increase the variable name before that line and use it with the new value.

But besides that I don't see a problem

Answer 4

I don't have a problem with what you're doing there, but I think you could use a comment to make it absolutely clear that you're looking for the last / and taking the rest of the string after that position.

Answer 5

you are not modifying the variable there.

To do that would be

 return path.substring(indexOfLastSeparator++);

or

 return path.substring(++indexOfLastSeparator);

Answer 6

As long as it is easy to read and understand the code, i don't see any problem with this technique. It might actually become less readable if you were to, say, increment the variable in an extra line.

Answer 7

You don’t modify the variable itself, you only modify the expression. And that’s completely okay, especially in this case as you want your new string to begin at the character after the position of the last separator.

Answer 8

If you want to go the whole way, I would do something like this:

    int indexOfLastSeparator = path.lastIndexOf('/');
    if (indexOfLastSeparator > -1)
    {
            int indexOfFirstCharInFilename = indexOfLastSeparator + 1;
            return path.substring(indexOfFirstCharInFilename);
    }
    else
    {
            return path;
    }

This is not yet with good performance, since a new variable is created, but it's highly readable in the way I think you expect it.

Personally I would prefer something like John Nolan suggested:

return path.substring(++indexOfLastSeparator);

It's not that chatty like you want it, but even a programming beginner gets the idea if he read the description of the substring method.

edit: Be aware, the use of "/" in the check of the path is not platform-independent. Use a lib function to get the system-dependent seperator if you're code shall be run on different platforms.

Answer 9

Actually in .net you should probably use Path.GetFilename(); instead.

Answer 10

It IS in GDI+ or drawing code.

Sometimes an underlying issue causes something to be off by a pixel or so and at this point you have two choices.

• Fix the underlying issue
• Make all further operations use this crappy +1 or -1 offset.

If the latter is chosen this code smell will be littered across the code.

Answer 11

There are two reasons to prefer writing a constant over a magic number:

1. If the value changes, refactoring can be a nightmare when using a magic number
2. Magic numbers decrease readability.

If you're sure that neither of these are an issue, I say go for it. In the case of the code you give as an example, I think that:

1. The offset of the path from the last separator is unlikely to change and
2. It's not terribly difficult to tell what's going on.

So I say that this is a valid use of a magic number. However, I would also point out that you do have to worry about these two issues more often than you don't, so when in doubt, use a constant.

Answer 12

In Java path.substring(n) does not modify path, or even the String object referenced by path.