1
\$\begingroup\$

My code was made as an practice for CodeChef. Here's the brief from that page:

Relational Operators are operators which check the relationship between two values.
Given two numerical values A and B you need to help chef in finding the relationship between them that is:
First one is greater than second or
First one is less than second or
First and second one are equal.

I couldn't submit the code because of an error so I'd like to see the criticism of the Code Review community.

package main;

import java.util.Scanner;

public class Method1 {
    public static float a, b, t;
    public static Scanner scanner;

    public static void main(String[] args) {
        scanner = new Scanner(System.in);
        System.out.println("Type your first Value please.");
        a = scanner.nextFloat();
        System.out.println("Type your second Value please.");
        b = scanner.nextFloat();
        if (a == b) {
            System.out.println('=');
        }
       if (a > b){
           System.out.println('>');
       }
       if (a < b){
           System.out.println("<");
       }
    }
}

I would like to note that I know its very basic and easy to make but its just a practice for me to see where I'm at in this kinda stuff.

\$\endgroup\$
2
  • 3
    \$\begingroup\$ Presumably codechef rejected this because this is not how it expects to give input. I think this is valid code which does what it's supposed to (unless I'm missing some bug), it's just that the description makes it sound like it's supposed to do something else. As long as TLDuys isn't expecting us to explain how to adapt it to codechef's input format, I don't think it's off-topic. \$\endgroup\$ – Ben Aaronson Sep 8 '15 at 15:40
  • 1
    \$\begingroup\$ How to ask a good question... and by "broken code", we don't just mean un-compilable code, as long as it gives the wrong answer, I think our preference is for OP to understand the coding challenge first, and then fix what needs to be done to pass it. Then we'll be glad to look into the finer details of the code... \$\endgroup\$ – h.j.k. Sep 8 '15 at 15:52
6
\$\begingroup\$
public class Method1 {

Since this will be the name of the file too, (if you save it) I would recommend naming it something descriptive so that you don't have to open it in order to know what it hides.


public static float a, b, t;
public static Scanner scanner;

Try and declare as close to the use of the variables as possible. There is also no need for these to be globals, since they are only used in one place.

Next, read the question very carefully. It states that A and B are both integers, so a float is the wrong type to use. It may also give rounding errors, and comparing floats is trickier because of how floating point numbers are stored. 0.1 + 0.1 does not always equal 0.2, it may give 0.200000001, so this can lead to a bug.

Lastly, t is also going to be an integer. You can tell this is the case because it is a count of how many test cases there are, 2.3 testcases doesn't make sense.


HINT TOWARDS AN ERROR

System.out.println("Type your first Value please.");

Why is this here? Does the question ask for it in the output?


Now onto the code overview. Strictly speaking, your code doesn't work at all. You never actually took in all the input. If its input looks like

3
10 20
20 10
10 10

You scan 3, and 10. That is it. All the rest of the input is ignored. There is a reason it tells you how many test cases there will be, its important to use that information.


if (a == b) {
    System.out.println('=');
}
if (a > b){
    System.out.println('>');
}
if (a < b){
    System.out.println("<");
}

This is the perfect time for if/else statements.

One good piece of logic here is if they aren't equal, and a isn't bigger than b, we know the answer to a < b without needing to check.

There are more things to mention, like moving the check if two numbers are equal to a method, and making use of the library of methods available, but get it working first, then post a follow up. Best of luck

\$\endgroup\$
3
\$\begingroup\$

Don't name anything Method1. It's never going to be a good name but in particular here you used it as the name of a class, which is extra confusing.

You should name a class after its use. Maybe something like OperatorChecker. A name should tell you what the class does, that makes it easier to understand when and how to use it.

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

Generally, this looks pretty good. There are a few points to make, but it's worth first saying that because this is such a simple problem and solution, some of these may seem a little pointless. I'll try to flag up when this is: even if they're pointless in this extremely simple case, things don't need to get much more complex before all of these are very important!


public class Method1

This is a bad name. A class isn't the same thing as a method, and also this name has no useful information.


public static float a, b, t;

What's t? I'm guessing this was just a mistake, but don't leave variables lying around that aren't being used.

It's worth noting that generally, you should give variables more descriptive names than this, but in purely mathematical contexts like this, sometimes a and b really are as descriptive as you can be about the role of a variable!


public static float a, b, t;
public static Scanner scanner;

Given that this program has a single class, and in fact a single method, it doesn't really matter where you declare things. However, in general if you're only using a variable inside a method, you should declare it in that method rather than at the class level like you have.

Even if you were using it throughout the class, you should still keep it private, not public, unless you explicitly want to expose it to other classes.


if (a < b){

This isn't needed, since you've already checked if it's greater than or equal, anything that's left over must be less! You can use an if{...}else if{...}else{...} here. Note that this one is more a matter of preference- arguably your version is more clearly readable than using elses would be. Either way though, it's important to understand both options.


Finally a higher-level point. This example mixes together what I'd call presentation concerns and business logic.

Usually those terms wouldn't be applied to such a small, straightforward application, but essentially, presentation is how you display and get input from the user, and business logic is the actual logical work that your application is doing.

Since your entire application is so simple, this probably doesn't matter much, but it's a good idea to start early with thinking in terms of how you're going to separate out responsibilities. A single method shouldn't be taking responsibility both for presentation concerns (deciding how to ask for the numbers and print the response) and business logic (in this case, doing the number comparison). Refactoring these into two different methods your class might look like:

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);
    System.out.println("Type your first Value please.");
    float a = scanner.nextFloat();
    System.out.println("Type your second Value please.");
    float b = scanner.nextFloat();

    string result = compare(a,b);
    System.out.println(result)
}

private static string compare(float a, float b) {
    if (a == b) {
        return "=";
    }
   if (a > b){
       return ">";
   }
   if (a < b){
       return "<";
   }
}

Notice how now, if you do want to modify your program to be a valid submission to CodeChef, you don't need to modify compare. It's only the input/output (presentation) that doesn't fit with CodeChef, the actual logical work your program is doing (the body of compare) is completely independent. You could even build a GUI over it and you still wouldn't need to change the contents of that method.

That may not seem important since compare is 9 lines of code, but imagine your program was doing something much more complex which requires 90 lines, or 900. Not having to modify every little bit of it just to do IO a little differently would be vital!

\$\endgroup\$
1
  • \$\begingroup\$ t is the number of testcases. The compare method is fine for ints, but is not great for floating point numbers \$\endgroup\$ – spyr03 Sep 8 '15 at 16:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy