Code smell? - Setting 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 this:

 return path.substring(indexOfLastSeparator + 1); 

Is it bad practice to modify an inline expression? Suggestions on how to reorganize to increase readability will be most welcome.

---- ---- Edit OK is updated after comments. Thanks everyone for the answer! I do not want to actually change the value of the variable at all, just the expression that it uses.

Another published one suggested that I could tear out this part of the expression, as shown in the second code snippet below. Better / worse / no difference? :) I'm starting to suspect that I'm too careful here.

 return path.substring(indexOfLastSeparator + 1); 

or

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

Answer 1

If you want to go all the 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, as a new variable is being created, but it is very readable the way I think you expect it.

Personally, I would prefer something like John Nolan:

 return path.substring(++indexOfLastSeparator); 

This is not as chatty as you want, but even a novice programmer gets an idea if he reads a description of a substring method.

edit: Remember that using "/" in path verification is platform independent. Use the lib function to get a system-specific delimiter if you use code on different platforms.

Answer 2

I do not understand why this would be bad practice. Perhaps you can increase the variable name to this string and use it with a new value.

But also, I do not see a problem

Answer 3

In this case, you can simply 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 link - not sure about .NET.

In general, however, there are many times when it makes sense to add or subtract one. I really don't think this is the smell of code.

Answer 4

I have no problem with what you do there, but I think you could use a comment so that it is absolutely clear that you are looking for the last / and leaving the rest of the line after this position.

Answer 5

In fact, in .net you should probably use Path.GetFilename(); .

Answer 6

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

• If the value changes, refactoring can be a nightmare when using a magic number
• Magic numbers reduce readability.

If you are sure that none of them is a problem, I say that you need to go for it. In the case of the code that you give as an example, I think that:

• The path offset from the last delimiter is unlikely to change and
• It’s not difficult to say what is happening.

Therefore, I say that this is the actual use of magic numbers. However, I would also like to point out that you have to worry about these two problems more often than you, so when in doubt, use a constant.

Answer 7

While it is easy to read and understand the code, I see no problems with this technique. This may become less readable if you want to, say, increment this variable in an extra line.

Answer 8

If you are using .NET, look at System.Io.Path.Combine (string, string) , which uses 2 lines to combine without checking and / or removing trailing slashes

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

Answer 9

you do not change the variable there.

For this you need

  return path.substring(indexOfLastSeparator++); 

or

  return path.substring(++indexOfLastSeparator); 

Answer 10

You do not change the variable yourself, you only modify the expression. And this is absolutely normal, especially in this case when you want your new line to start with a character after the position of the last separator.

Answer 11

He is in GDI + or drawing code.

Sometimes the main problem causes something to turn off a pixel or so, and at this point you have two options.

• Fix the main question
• To do all subsequent operations use this shitty shift of +1 or -1.

If the latter is selected, this code will be clogged through the code.

Answer 12

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