5
\$\begingroup\$

I have written a code for following problem:

Write a program to grade a short multiple-choice quiz. The correct answers for the quiz are:

  1. C 5. B
  2. A 6. C
  3. B 7. C
  4. D 8. A

Assume that the pass marks are 5 out of 8. The program stores the correct answers in an array. The submitted answers are specified as program arguments. Let X represent a question that was not answered on the quiz. Use an enum type to represent the result of answering a question. The program calculates and prints a report along the following lines:

enter image description here

And this is my program:

public class Grades {

    enum Result {
        CORRECT, WRONG, UNANSWERED;
    }

    private static String[] correctAnswers = {"C","A","B","D","B","C","C","A"};
    private static int correct,wrong,unanswered;

    public static void main (String[] answers) {
        String[] args = answers[0].split(",");
        if(null == args) {
            System.out.println("No questions answered.");
            System.exit(-1);
        } else if(8 > args.length) {
            System.out.println("You must answer all questions. Or write X for the question, you don't want to answer.");
            System.exit(-1);
        }else if(8 < args.length) {
            System.out.println("There are only 8 questions.");
            System.exit(-1);
        }

        showReport(args);
    }


    private static void showReport(final String[] args) {
        System.out.println("Question\tSubmittedAns.\tCorrectAns.\tResult");
        for(int i=0;i<args.length;i++) {
            System.out.println(i+1+"\t\t"+args[i]+"\t\t"+correctAnswers[i]+"\t\t"+evaluateAnswer(args[i],i));
        }

        System.out.println("No. of correct answers: "+correct);
        System.out.println("No. of wrong answers: "+wrong);
        System.out.println("No. of correct answers: "+unanswered);

        if(5 < correct) {
            System.out.println("The candidate PASSED.");
        } else {
            System.out.println("The candidate FAILED.");
        }

    }

    private static Result evaluateAnswer(String answer, int index) {

            if(answer.equalsIgnoreCase(correctAnswers[index])) {
                correct++;
                return Result.CORRECT;
            } else if(answer.equalsIgnoreCase("X")) {
                unanswered++;
                return Result.UNANSWERED;
            } else {
                wrong++;
                return Result.WRONG;
            }



    }

}

Any suggestions, how can I improve in terms of coding strategies, coding standards etc.

\$\endgroup\$
8
\$\begingroup\$

Most of this seems good, so the points are relatively minor.

Conditional order

Your conditional statements are written like null == args or 8 < args.length. This style, with the variable on the right, is a trick from C/C++ designed to prevent accidentally typing args = null and doing assignment rather than comparison. However, in Java, there's no need to do this, because if you typed args = null inside a conditional statement, you'd get a compiler error.

Using a C idiom in Java is confusing, and means that any Java developer reading your code is likely to have a bit of extra cognitive friction before they can parse what's going on. That extra difficulty in translating between code and what you actually want to say leads to bugs, like:

if(5 < correct) {

Which should actually (in this style) be 5 <= correct. I'd guess you'd be less likely to make that mistake if you wrote it Java-idiomatically: correct >= 5.

Side-effects

Incrementing correct, unanswered and wrong inside evaluateAnswer seems like a pretty clever way of cutting down on the code required to get the final counts. In this case, with a relatively small project and simple problem, it possibly is.

However, in general, even for projects only slightly more involved than this one, things like that are often only clever up until they're suddenly not. This kind of tight-coupling becomes a problem if you need to refactor, or adapt to requirement changes. It also definitely violates the Principle of Least Surprise: evaluateAnswer has a side-effect of changing a static variable, which you would not expect without reading it.

Generally, a method is either expected to return the answer to a question or change some state. Sometimes both, never neither. It's easy to tell whether a method returns the answer to a question because it's not void. But if it's also going to change state, you want to make sure that's very clear from its signature alone. Otherwise it's easy to make assumptions about it which aren't true- for example, that calling it twice will always be equivalent to calling it once.

In this case, while it's a few extra lines of code, I'd suggest adding an extra private static void addResultToCount(Result result) method, and call that inside the showReport for loop. This separates out your concerns nicely and makes it much clearer what's going on where. Then those two methods would look something like:

private static Result evaluateAnswer(String answer, int index) {

        if(answer.equalsIgnoreCase(correctAnswers[index])) {
            return Result.CORRECT;
        } else if(answer.equalsIgnoreCase("X")) {
            return Result.UNANSWERED;
        } else {
            return Result.WRONG;
        }
    }

private static void addResultToCount(Result result) {
        switch(result) {
            case CORRECT:
                correct++;
                break;
            case WRONG:
                wrong++;
                break;
            case UNANSWERED:
                unanswered++;
                break;
        }
    }

Or similar (apologies for any syntax errors with the above, Java isn't my main language)

Expressive names

The names are generally good, but there are places where they could be more expressive. In main, args and answers seem like they should be the other way around. Once those get passed to showReport, they should definitely be interpreted as the more meaningful answers.

Similarly, index in evaluateAnswer doesn't tell us much. I'd suggest questionNumber, but this would be a bit confusing between your 0-indexed array and the 1-indexed actual question numbers. So questionIndex would probably be a good compromise.

Magic numbers

Another minor one for this particular project, but generally a good habit to get into. Instead of hard-coding magic numbers 8 and 5 into your methods, these should be put into constants with expressive names.

In fact, the 8 could instead be taken from the length of correctAnswers, so that you're not expressing the same information in two different places.

\$\endgroup\$
  • \$\begingroup\$ I am really thankful to you @Ben for taking out time to evaluate this and surely, I will keep in mind all of your instructions and will implement in the current example too. Once again, thanks a lot for your time. \$\endgroup\$ – D D Nov 20 '14 at 12:51
  • \$\begingroup\$ I am confused about addToResultCount(Result result) function, you just mentioned, what exactly you meant by that? \$\endgroup\$ – D D Nov 20 '14 at 12:56
  • 1
    \$\begingroup\$ @Mr.777 You're welcome! As for the second comment, I've added a snippet to explain. There are probably even better ways to do this, this was just the simplest given the current code. \$\endgroup\$ – Ben Aaronson Nov 20 '14 at 14:03
2
\$\begingroup\$

I'd like to point out another minor issue: args will never be null, instead they will have length 0 if no arguments are provided.

So your first check should be args.length == 0

\$\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.