4
\$\begingroup\$

Looking for reviewers to suggest optimization and improvements, and to evaluate coding practices.

final class Variables {
    private final int x;
    private final int y;

    Variables(int x2, int y2) {
        this.x = x2;
        this.y = y2;
    }

    public int getX() {
        return x;
    }

    public int getY() {
        return y;
    }

    @Override public String toString()  { 
        return "x = " + x + " y = " + y;
    }
}

public final class FindMissing {

    private FindMissing() {};

    /**
 * Takes an input a sorted array with two variables that repeat in the
 * range. Remaining variable are consecutive. and returns two variables that repeats
 * 
 * eg: for a range of 1,2,3,4, a valid input is [1, 4], [1, 3] etc.
 * 
 * @param a : the sorted array.
 */
/*
 * Why is start needed ?
 * If start is not given then if a1 is empty how do we know if missing numbers were
 * [1, 2] or [4, 5] ??. 
 */
public static Variables sortedConsecutiveTwoMissing (int[] a, int startOfRange) {
    if (a == null) throw new NullPointerException("a1 cannot be null. ");

    int low = startOfRange;
    // calculating the highest value of the range. it needs to be 2 more than length since all elememts are consecutive.
    int high = startOfRange + (a.length - 1) + 2;

    int i = 0;
    boolean foundX = false;
    int x = 0; 

    while (i < a.length) {
        if (a[i] != low) {
            if (foundX) {
                return new Variables(x, low);
            } else {
                x = low;
                foundX = true;
            }
        } else {
            i++;
        }
        low++;
    }

    return new Variables(x, high);
 }

public static void main(String[] args) {
        // unsortedConsecutiveTwoMissing
        int[] a6 = {1, 4};
        System.out.println("Expected 2 and 3, Actual " + sortedConsecutiveTwoMissing(a6, 1));
        // unsortedConsecutiveTwoMissing
        int[] a7 = {-4, -1};
        System.out.println("Expected -3 and -2, Actual " + sortedConsecutiveTwoMissing(a7, -4));
        int[] a8 = {-4, -3, -2, -1};
        System.out.println("Expected 0 and 1, Actual " + sortedConsecutiveTwoMissing(a8, -4));
        int[] a9 = {5};
        System.out.println("Expected 4 and 6, Actual " + sortedConsecutiveTwoMissing(a9, 4));
        }
     }
\$\endgroup\$
  • \$\begingroup\$ I am not convinced that this code works.... low is initialized to an index in the array, but then it is compared to a value in the array if (a1[i] != low). This makes no sense... is low an index, or a value? Can it possibly be both? \$\endgroup\$ – rolfl Nov 25 '13 at 3:45
  • \$\begingroup\$ @rolfl: Start is not an index. It's the start of the value range (like the smallest number in the range) \$\endgroup\$ – ChrisWue Nov 25 '13 at 5:50
  • 1
    \$\begingroup\$ Can you provide us some test cases. I think for some cases your code will not behave as like you think. Specially how are you invoking the method, I mean the parameters. \$\endgroup\$ – Anirban Nag 'tintinmj' Nov 25 '13 at 9:44
  • 1
    \$\begingroup\$ This code is buggy, provided the method is supposed to do what its name says it does: "x = 6 y = 6".equals(FindMissing.sortedConsecutiveTwoMissing(new int[]{5}, 4).toString()) \$\endgroup\$ – abuzittin gillifirca Nov 25 '13 at 14:38
  • \$\begingroup\$ @tintinmj provided test cases \$\endgroup\$ – JavaDeveloper Nov 26 '13 at 3:49
1
\$\begingroup\$

You say the problem is to find missing values from a 'Range', but your method only takes a start value, not an end value.... is this intentional? The method comment says:

Takes an input a sorted array with two variables that repeat in the range. and returns two variables that repeats

The method does nothing of the sort though... it takes one variable, and there's no indication the values repeat, and it returns missing values, not repeats.

Additionally, the method name is sortedConsecutiveTwoMissing but there's nothing in the implementation that suggests that the values have to be consecutive at all...

Also you suggest the array is sorted, but not whether it is unique... for example, your code may break on (missing is 3 and 5):

0 1 2 2 4 4 6 6 7 8 9

General / Code Style

  • Variables like a1 make me want to look for a2, a3, etc. Unless it's obvious, don't use numbered variables.
  • int high = start + (a1.length - 1) + 2; should have a comment .... the + 2 is not obvious, and magic numbers should either be very obvious, or documented. That line still makes no sense to me at all.... and high is not used for anything reasonable anyway.
  • calculatedX is not a calculated anything

At this point I don't believe the code does what you suggest it should, so I don't believe the code works... downvote time.

Edit the code, make it work....

\$\endgroup\$
  • \$\begingroup\$ range takes start - yes its intentional. end or highbound can be computed from arraysize and knowledge that 2 elements are missing \$\endgroup\$ – JavaDeveloper Nov 25 '13 at 7:57
  • \$\begingroup\$ nothing of the sort though - javadoc make it very clear that function expects sorted array \$\endgroup\$ – JavaDeveloper Nov 25 '13 at 7:58
  • \$\begingroup\$ nothing in the implementation that suggests that the values have to be consecutive at all...-- what do u suggest i should do ? \$\endgroup\$ – JavaDeveloper Nov 25 '13 at 8:00
  • \$\begingroup\$ for example, your code may break on (missing is 3 and 5): -- the code demands that only 2 variables should repeat. if 3 or more repeat then its breaking function contract. Please lets assume input is sorted consecutive with only two elements occuring twice and give feedback \$\endgroup\$ – JavaDeveloper Nov 25 '13 at 8:01
  • 1
    \$\begingroup\$ Down-vote now un-down-voted. \$\endgroup\$ – rolfl Nov 26 '13 at 3:58

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.