3
\$\begingroup\$

Given two int values, print whichever value is larger. However if the two values have the same remainder when divided by 5, then the print the smaller value. However, in all cases, if the two values are the same, print 0.

This is what I have so far:

public static void main(String[] args)
{
    Scanner scan = new Scanner(System.in);

    int x = scan.nextInt();
    int y = scan.nextInt();

    if(x > y || (x%5 == y%5 && y > x))
    {
        System.out.println(x);
    }
    else if (y > x || (x%5 == y%5 && x > y ))
    {
        System.out.println(y);
    }
    else if(x == y)
    {
        System.out.println("0");
    }
}

My score was 79/100 when I sent it to an online judge

What seems to be the problem?

\$\endgroup\$
  • \$\begingroup\$ Off the top of my head: you didn't abstract this out to a method, and you assumed you would be getting the variables from Scanner input. That might warrant lower points. \$\endgroup\$ – 410_Gone Oct 20 '15 at 19:54
  • \$\begingroup\$ You should ask yourself how you would write a unit test with this code? You can't do you? You mix IO operation with algorithm, it's really bad, it doesn't deserve 79 :p You should always send unit test with that kind of assignment, even when it is not explicitly asked, it's always bonus points. \$\endgroup\$ – Cyril Gandon Oct 21 '15 at 12:00
  • \$\begingroup\$ Apart from questions of readability, the code doesn't implement the specification. E.g. if x=10 and y=5 it will print x (by the first rule) whereas it should print y (by the second rule). \$\endgroup\$ – Joe Lee-Moyet Oct 21 '15 at 12:14
10
\$\begingroup\$
  • What condition is the most important?

    It seems like if (a == b) is the most important, which is what you then should handle first and use else for the rest.

  • Duplicated logic

    You have written (x%5 == y%5 && y > x) twice in your code.

  • Using Math.min or Math.max

    Instead of using if (x > y) and if (x < y) you can let Java find out what is smaller or larger.

    public static void main(String[] args) {
        Scanner scan = new Scanner(System.in);
    
        int x = scan.nextInt();
        int y = scan.nextInt();
    
        if (x == y) {
            System.out.println(0);
        } else if (x % 5 == y % 5) {
            System.out.println(Math.min(x, y));
        } else {
            System.out.println(Math.max(x, y));
        }
    }
    

Note: I can't guarantee that this will give you 100 points. I can't even guarantee that this will give you more points than Caridorc's solution with ternaries. But in my opinion, this is the better solution.

\$\endgroup\$
  • \$\begingroup\$ Use return early and often instead of else. Put the code into its own method where the println()s are replaced with return statements. This makes the code easier to follow and reduces/eliminates subsequent indentation. Code becomes: public static int function(final int x, final int y) { if (x == y) return 0; if (x % 5 == y % 5) return Math.min(x, y); return Math.max(x, y); } and then in main(): System.out.println(function(x,y)); \$\endgroup\$ – Harvey Oct 21 '15 at 14:25
7
\$\begingroup\$

One of the problem, I think, is that you should check if both number are equals before doing anything else. Why bother doing arithmetic computing (in your ifs) for nothing?

Also, you should have a method that solves your problem, and do the IO in the main method, not all-in-one.

But the main problem is that you repeat your conditions. Here x > y is written twice :

if(x > y || (x%5 == y%5 && y > x))

And you repeat x%5 == y%5 twice.

You should first check if their modulos are equal, then check for which is bigger. Which gives something like this :

//Naming is hard in such a case.
private static int computeStuff(int x, int y) {
    if(x == y) {
        return 0;
    }
    if (x % 5 == y % 5) {
        return Math.min(x,y);
    }
    return Math.max(x,y);
}

public static void main(String[] args)
{
    Scanner scan = new Scanner(System.in);

    int x = scan.nextInt();
    int y = scan.nextInt();

    System.out.println(computeStuff(x,y));
}

(Note also the spacing is slightly clearer now)

This way, your computeStuff method has its own responsability and using return makes it clearer (in my opinion). The main method takes care of IO!

\$\endgroup\$
  • \$\begingroup\$ As @Simon said, I don't know if this would give you 100%, but it should yield a better score :) \$\endgroup\$ – IEatBagels Oct 20 '15 at 20:04
3
\$\begingroup\$

Probably it's because of duplicated expressions:

x%5 == y%5

These modulos are more expensive and are evaluated twice if x and y are equal, which means that modulo operation is unneccesary.

\$\endgroup\$
2
\$\begingroup\$

This is surely going to be controversial, but I suggest a nested ternary.

  • This uses less code, such a trivial problem does not need so much code in my opinion.

  • Ternaries are expressions, that are much easier to reason about than statements.

  • This is easier to change, as just what is needed is written, avoiding for example the repetition of System.out.println

  • This is faster, as the compiler is very good at optimizing ternaries.

  • This uses the built-ins Math.min and Math.max to simplify the code and make it nicely fit inside a ternary.

System.out.println( x == y ? 0 :
                      x % 5 == y % 5 ? Math.min(x, y) :
                        Math.max(x, y) );
\$\endgroup\$
  • 4
    \$\begingroup\$ You were right, it is controversial! :) \$\endgroup\$ – Simon Forsberg Oct 20 '15 at 19:54
  • 1
    \$\begingroup\$ @Caridorc In that case a method might be more suitable. ;) \$\endgroup\$ – 410_Gone Oct 20 '15 at 20:02
  • 2
    \$\begingroup\$ I think @Caridorc just loves the one-liners too much :p \$\endgroup\$ – IEatBagels Oct 20 '15 at 20:08
  • 2
    \$\begingroup\$ @SimonForsberg, I'm not really sure what your problem with the ternary is. It is concise and quite readable in this case, far more so than the thicket of brackets and wasted lines that is the if else solution. What everyone else does, or what the standard Java style guide says to do, is not always best. \$\endgroup\$ – Jonah Oct 20 '15 at 20:09
  • 1
    \$\begingroup\$ ChatRoom if anyone is pro or opposite to ternaries -> chat.stackexchange.com/rooms/30509/ternary-vs-if subjective discussions are better in chat. \$\endgroup\$ – Caridorc Oct 20 '15 at 20:15
2
\$\begingroup\$

Two numbers have the same remainder when divided by 5 iff their difference is divisible by 5. So you can save yourself one % operation (which is expensive!) by simply doing:

public static int result(int x, int y) {
    if (x == y) {
        return 0;
    }
    else if ((x-y) % 5 == 0) {
        return Math.min(x, y);
    }
    else {
        return Math.max(x, y);
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Remainder and modulus is not the same thing. -1 % 5 = -1, -1 mod 5 = 4. \$\endgroup\$ – Johnbot Oct 20 '15 at 21:56
  • \$\begingroup\$ @Johnbot I don't understand what your point is. There is no mod function. \$\endgroup\$ – Barry Oct 20 '15 at 22:11
  • 1
    \$\begingroup\$ 3 % 5 = 3, Integer.MIN_VALUE % 5 = -3 but (3 - Integer.MIN_VALUE) % 5 = 0. \$\endgroup\$ – Johnbot Oct 20 '15 at 22:19
  • 1
    \$\begingroup\$ @Johnbot Perhaps the reason OP got 79/100 is because the grader isn't using Java's definition of modulo? \$\endgroup\$ – Barry Oct 20 '15 at 23:15
  • \$\begingroup\$ Well this answer gets the min of x and y when they are congruent modulo 5 so this in not the same as the OP code. Yours is equivalent to using Math.floorMod( x, 5 ) == Math.floorMod(y, 5) and (x - y) may overflow. \$\endgroup\$ – MAG Oct 21 '15 at 0:25

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.