0
\$\begingroup\$

Given 2 values and an array, I must return the minimal distance between the indices of both values, as efficiently as I can in terms of run time and space.

This is my solution:

  public static int minDist (int[] a, int x, int y) {
    int index_one = a[0];
    int index_two = a[0];
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));
 
} 

I'm not sure it works, especially for extreme cases, but I thought the best way is to run the loop one time and find the index of both numbers.

Can I make it more efficient?

\$\endgroup\$
6
  • 4
    \$\begingroup\$ The code finds some distance between x and y. It is not necessarily minimal. \$\endgroup\$ Commented Nov 21, 2021 at 18:54
  • 4
    \$\begingroup\$ If you are unsure how well it works, perhaps you should include your unit tests in the question, so it's clear how much of it is known to work, and how much is uncertain. And is that the real layout of your code? Consider re-indenting before asking for review. \$\endgroup\$ Commented Nov 22, 2021 at 7:47
  • 1
    \$\begingroup\$ I don't think this code works even for the straightforward cases. I'm pretty sure you've fallen prey to the "no braces = one line" gotcha. I think you think that the return only triggers when the Y value is found. This code will always return 0 because it will always hit the return on the first iteration. CodeReview expects that posted code works as intended, which this doesn't. \$\endgroup\$ Commented Nov 22, 2021 at 14:30
  • 1
    \$\begingroup\$ @Flater, the indentation is misleading, as I mentioned. The return is 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\$ Commented Nov 23, 2021 at 6:22
  • \$\begingroup\$ @TobySpeight: Very good catch, seems like I fell pray to the same classic blunder because I missed it on the for. \$\endgroup\$ Commented Nov 23, 2021 at 8:57

1 Answer 1

1
\$\begingroup\$

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.

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.