This code does not do what you want it to do, there is no check for the minimum distance. I'm not going to review code that does not exist. However, I will review the rest of the code.
Naming conventions
- Variable names should be descriptive.
int[] a does not pass that requirement. numbers, values or even just arr would've been better.
- Note that
x and y are acceptable here, because the context of the method makes it clear that two numbers are needed, and there is no clear descriptive name available for these numbers.
- Try to avoid abbreviations and shortenings in method names.
minDist should have been GetMinimumDistance.
- Why use
x/y and then use index_one/index_two? Since the index variables specifically refer to the x/y values, index_X and index_Y would've been better.
Unbraced blocks
The major downfall of this code is the lack of braces. While the compiler does allow it, and IMHO there are some cases where a brace-less block is acceptable, this is not one of those cases. You've chained three brace-less blocks (for, if and if), which I consider to never be a valid case, as it starts becoming more of a decoding challenge for the reader than a coding challenge for the writer.
Your indentation on the return further misleads the reader, as it gets indented together with the else body. Not only does this suggest to a reader that the return happens inside the else, it also suggests that you think it does.
Rewritten:
for(int i = 0 ; i < a.length ; i++) {
if(a[i] == x) {
index_one = i;
}
else if(a[i] == y) {
index_two = i;
}
}
return Math.abs(index_one - index_two);
This syntax makes the intended flow much clearer to be parsed by a reader.
Mixing values and indices
int index_one = a[0];
int index_two = a[0];
I understand that you wanted your variables to have a default value, but there is a difference between an array element and an array index. While in this case both are numeric in nature, they should rarely if ever cross-contaminate.
Since these variables store indices, it makes no sense to give them the default value of the first element of the array.
I assume you wanted to do this:
int index_one = 0;
int index_two = 0;
However, I also disagree with choosing a valid index value here. The problem is that when you only find one (e.g. X) and not the other (e.g. Y), that you then end up returning the index of the found number, not the distance between the two.
That return value appears to be valid (to the caller) but is totally nonsensical and does not indicate distance whatsoever.
A more conventional approach here would be to return -1 in all cases where a distance cannot be computed (i.e. at least one of the items was not found). This is similar to the indexOf method, which returns -1 when the searched for element cannot be found.
-1 is a great return code here, because it cannot possible be a correct distance value, since distances are inherently positive numerical values.
However, now we get back to you using 0 as the default value for your index variables. Because of this, it is impossible to know whether the algorithm actually found a match (on index 0) or not (and 0 is still the default value). This is no good, because you make it impossible to know the outcome of your own algorithm. It literally defeats its own purpose.
We can apply the same -1 trick here. -1 is a better default value, because it clearly indicates that nothing has been found. If, after the for, either index variable is still set to -1, we know for a fact that the corresponding number was not found in the array, and therefore we immediately return -1 to indicate that no distance could be calculated.
Taking all the above review into account, a reworked version of your code would be:
public static int GetMinimumDistance(int[] values, int x, int y) {
int index_X = -1;
int index_Y = -1;
for(int i = 0 ; i < values.length ; i++) {
if(values[i] == x) {
index_X = i;
}
else if(values[i] == y) {
index_Y = i;
}
}
if(index_X == -1 || index_Y == -1) {
return -1;
}
return Math.abs(index_X - index_Y);
}
Just to reiterate, I did not add nor review any "minimum distance" logic, as this was missing from your version as well.
As an aside, there are existing ways to find the index of an element in an array. You might want to look into those. As a C# dev I'm surprised that Java does not have a native (and elegant) way of doing this.
xandy. It is not necessarily minimal. \$\endgroup\$returnonly triggers when the Y value is found. This code will always return0because it will always hit thereturnon the first iteration. CodeReview expects that posted code works as intended, which this doesn't. \$\endgroup\$returnis after the loop, so it will return the difference between the last X and Y values, by my reasoning. Still not the least difference, and completely weird (meaningless) result if either is not found. \$\endgroup\$for. \$\endgroup\$