8
\$\begingroup\$

I am practicing some simple Java coding problems in an attempt to learn while doing them. I would like to know if my code is redundant/is there an easier way to accomplish the same thing?

Question: Which Alien?

"A person who witnessed the appearance of the alien has come forward to describe the alien's appearance."

The program will determine which alien has arrived. The three alien species that it could be is:

  • TroyMartian, has at least 3 antenna and at most 4 eyes

  • VladSaturnian, has at most 6 antenna and at least 2 eyes

  • GraemeMercurian, has at most 2 antenna and at most 3 eyes

Sample session (with output shown in text, user input in italics)

How many antennas?

2

How many eyes?

3

VladSaturnian

GraemeMercurian

If the description does not match any of the aliens, there is no output.

My code:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;

public class Alien {

public static int antenna;
public static int eye;

public static void main(String args[]) {
    try (BufferedReader in = new BufferedReader(new InputStreamReader(
            System.in))) {

        System.out.println("How many antennas?");

        antenna = Integer.parseInt(in.readLine());

        System.out.println("How many eyes?");

        eye = Integer.parseInt(in.readLine());

        if(troy(antenna, eye)) {
            System.out.println("TroyMartian");
        }

        if(vlad(antenna, eye)) {
            System.out.println("VladSaturnian");
        }

        if(graeme(antenna, eye)) {
            System.out.println("GraemeMercurian");
        }

        return;

    }

    catch (IOException e) {
        System.err.println("Error");
    }

}

public static boolean troy(int antenna, int eye) {
    if ((antenna >= 3) && (eye <= 4)) {
        return true;
    } else {
        return false;
    }

}

public static boolean vlad(int antenna, int eye) {
    if ((antenna <= 6) && (eye >= 2)) {
        return true;
    } else {
        return false;
    }

}

public static boolean graeme(int antenna, int eye) {
    if ((antenna <= 2) && (eye <= 3)) {
        return true;
    } else {
        return false;
    }

}
}
\$\endgroup\$
6
\$\begingroup\$

You're on the right track. Good job using the try-with-resources block for the BufferedReader. You could use a Scanner instead for convenience. (You could even use Scanner.nextInt(), but it would work slightly differently: the newline in the input would be optional.)

The variables antenna and eye can be local to main(), and therefore should not be static members of the class.

In Java, a common naming convention for methods that return a boolean is isSomething() or hasSomething(). In this case, though, maybeSomething() seems more appropriate.

It is rarely necessary to write return true; or return false; explicitly. Usually, you would be better off returning a boolean expression.

import java.util.Scanner;

public class Alien {
    public static void main(String args[]) {
        try (Scanner in = new Scanner(System.in)) {
            System.out.println("How many antennas?");
            int antenna = Integer.parseInt(in.nextLine());

            System.out.println("How many eyes?");
            int eye = Integer.parseInt(in.nextLine());

            if (maybeTroy(antenna, eye)) {
                System.out.println("TroyMartian");
            }

            if (maybeVlad(antenna, eye)) {
                System.out.println("VladSaturnian");
            }

            if (maybeGraeme(antenna, eye)) {
                System.out.println("GraemeMercurian");
            }
        }
    }

    public static boolean maybeTroy(int antenna, int eye) {
        return ((antenna >= 3) && (eye <= 4));
    }

    public static boolean maybeVlad(int antenna, int eye) {
        return ((antenna <= 6) && (eye >= 2));
    }

    public static boolean maybeGraeme(int antenna, int eye) {
        return ((antenna <= 2) && (eye <= 3));
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When you return a boolean expression, does it just test if whatever is in the bracket is true or false? And then if it is true then that boolean will be true and vice versa for false? \$\endgroup\$ – quidproquo Jan 31 '15 at 5:33
  • \$\begingroup\$ Exactly, thats what it does \$\endgroup\$ – Ahkam Nihardeen Jan 31 '15 at 5:35
  • \$\begingroup\$ It works just like your original code. An expression like antenna >= 3 evaluates to either true or false. You can further combine such expressions — a && b would also evaluate to true or false. \$\endgroup\$ – 200_success Jan 31 '15 at 5:36
  • 1
    \$\begingroup\$ I've always been a fan of Console for reading input. Mainly because it provides an easy way to prompt the user. \$\endgroup\$ – Boris the Spider Jan 31 '15 at 8:54
  • \$\begingroup\$ @BoristheSpider Note that Console will not work with redirected input, which is often how online judges work. The input really does have to come from a TTY, not a stream. \$\endgroup\$ – 200_success Jan 31 '15 at 9:22
6
\$\begingroup\$

I like 200_success's solution. As an addition, if you are using "fancy" language features, you might as well make the last for loop more expressive by using a Stream with filter():

Stream.of(Alien.values())
    .filter(alien -> alien.maybe(antenna, eye))
    .forEach(System.out::println);

Also, when I read alien.maybe(antenna, eye)) I get the feeling this is something that becomes less readable when you return to your code later. Depends on your taste in method names, I guess. I myself would call it species.matchesDescription(antennae, eyes).

Since selecting by number of antennae and eyes is logic that resides inside the Alien enum and the enum itself knows all possible instances (which is actually similiar to a kind of collection), it might be better to put the selection logic inside that class and not in the main method. This is also a good example for the "Tell, don't ask" principle. The advantages of this might not be so obvious in small code like this (especially because here it actually adds to the line count, rather than make it shorter), but it is always important to think about where to put logic.

In this case, I think of two different parts. One is your application which talks to the user and stores user input.

The other part is your domain logic for which you have the Alien class (or enum), which would make sense even without your main application. This is why I would create a method inside Alien that selects all matching alien species rather than do that myself in the main method (i.e. the application).

Applying that to 200_success's code, this is what I came up with:

import java.util.Scanner;
import java.util.function.BiPredicate;
import java.util.stream.Stream;

public enum AlienSpecies {
    TroyMartian((Integer antennae, Integer eyes) -> antennae >= 3 && eyes <= 4),
    VladSaturnian((Integer antennae, Integer eyes) -> antennae <= 6 && eyes >= 2),
    GraemeMercurian((Integer antennae, Integer eyes) -> antennae <= 2 && eyes <= 3);

    private BiPredicate<Integer, Integer> criteria;

    AlienSpecies(BiPredicate<Integer, Integer> criteria) {
        this.criteria = criteria;
    }

    public boolean matchesDescription(int antennae, int eyes) {
        return this.criteria.test(antennae, eyes);
    }

    public static Stream<AlienSpecies> allMatchingDescription(int antennae, int eyes) {
        return Stream.of(values())
                .filter(species -> species.matchesDescription(antennae, eyes));
    }
}


public class Main {
    public static void main(String args[]) {
        try (Scanner in = new Scanner(System.in)) {
            int antennae = readIntFrom(in, "How many antennas?");
            int eyes = readIntFrom(in, "How many eyes?");

            AlienSpecies.allMatchingDescription(antennae, eyes)
                    .forEach(System.out::println);
        }
    }

    private static int readIntFrom(Scanner in, String message) {
        System.out.println(message);
        return Integer.parseInt(in.nextLine());
    }
}

Again, this might look a bit more complicated at first, but seperating concerns and avoiding code duplication will make your programs easy to extend and to change. Although it is not very likely that you'll want to extend this program later, I think it is still a good exercise to look for these things and think about where to put logic all the time.

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

One minor remark about your implementation is that the identifying test for each alien is separated from the name. For a simple program like this, it's no big deal. In a larger program, it could be a bit of a code maintenance headache.

One solution, presented below, uses enums and lambda expressions to centralize the descriptions of the alien types. There are other solutions that don't involve such fancy language features; I'll leave it up to you to think about.

import java.util.Scanner;
import java.util.function.BiPredicate;

public enum Alien {
    TroyMartian    ((Integer antenna, Integer eye) -> antenna >= 3 && eye <= 4),
    VladSaturnian  ((Integer antenna, Integer eye) -> antenna <= 6 && eye >= 2),
    GraemeMercurian((Integer antenna, Integer eye) -> antenna <= 2 && eye <= 3);

    private BiPredicate<Integer, Integer> criteria;

    Alien(BiPredicate<Integer, Integer> criteria) {
        this.criteria = criteria;
    }

    public boolean maybe(int antenna, int eye) {
        return this.criteria.test(antenna, eye);
    }

    public static void main(String args[]) {
        try (Scanner in = new Scanner(System.in)) {
            System.out.println("How many antennas?");
            int antenna = Integer.parseInt(in.nextLine());

            System.out.println("How many eyes?");
            int eye = Integer.parseInt(in.nextLine());

            for (Alien alien : Alien.values()) {
                if (alien.maybe(antenna, eye)) {
                    System.out.println(alien);
                }
            }
        }
    }
}
| improve this answer | |
\$\endgroup\$

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.