8
\$\begingroup\$

Are there any ways I can make my program more efficient? My program runs, but are there any things that are unneeded that I included?

The local Driver's License Office has asked you to write a program that grades the written portion of the license exam. The exam has 20 multiple choice questions. Here are the correct answers:

1. B    2. D    3. A    4. A
5. C    6. A    7. B    8. A
9. C    10. D   11.B    12. C
13. D   14. A   15. D   16. C
17. C   18. B   19. D   20. A

A student must correctly answer 15 questions of the 20 questions to pass the exam. Write a class named DriverExam that holds the correct answers to the exam in an array field. The class should have an array field to hold the student's answers. The class should have the following methods:

passed: The method returns true if the student passed the exam, false otherwise totalCorrect: returns the total number of correctly answered questions totalIncorrect: returns the total number of incorrectly answered questions questionsMissed: an int array containing the question numbers of the question that the student missed

Demonstrate the class in a test program that asks the user to enter a student's answers, and then display the results returned from the DriverExam class's methods. Input validation: only accept the letters A, B, C, or D as answers


    public class DriverExam
    {
       //An array containing a student's answers
       private String[] correctAnswers = 
                                     {"B", "D", "A", "A", "C", "A", 
                                      "B", "A", "C", "D", 
                                      "B", "C", "D", "A", 
                                      "D", "C", "C", "B", "D", "A"}; 

       //Store the user's answers
       private String[] userAnswers; 
       int[] missed = new int[correctAnswers.length]; 

       //Process the user's answers
       public DriverExam (String[] Answers)
       {
          userAnswers = new String[Answers.length]; 

          for (int i = 0; i < Answers.length; i++)
          {
             userAnswers[i] = Answers[i]; 
          }
       }

       //Returns a boolean value if correct answers > 15 
       public boolean passed()
       {
          if (totalCorrect() >= 15)
             return true; 
          else
             return false; 
       }

       //Determines the total correct answers
       public int totalCorrect()
       {
          int correctCount = 0; 

          for (int i = 0; i < correctAnswers.length; i++)
          {
             if (userAnswers[i].equalsIgnoreCase(correctAnswers[i]))
             {
                correctCount++; 
             }
          }
          return correctCount; 
       }

       //Determines the total incorrect answers
       public int totalIncorrect()
       {
          int incorrectCount = 0; 

          for (int i = 0; i < correctAnswers.length; i++)
          {
             if (!userAnswers[i].equalsIgnoreCase(correctAnswers[i]))
             {
                missed[incorrectCount] = i; 
                incorrectCount++; 
             }
          }
          return incorrectCount; 
       }

       //Missed questions
       public int[] questionsMissed()
       {
          return missed; 
       }

    }
    //end of DriverExam class

   /* The DriverExamApplication class demonstrates the methods of DriverExam class. */


import java.util.Scanner; 

public class DriverExamApplication
{
   public static void main(String[] args)
   {
      System.out.println("    Driver's License Exam "); 
      Scanner input = new Scanner(System.in); 

      System.out.println(" 20 Multiple-Choice Questions "); 
      System.out.println("       Mark A, B, C, D   "); 

      //Inputting string
      String[] answers = new String[20]; 
      String answer; 

      for (int i = 0; i < 20; i++)
      {
         do
         {
            System.out.print((i+1) + ": "); 
            answer = input.nextLine(); 
         } while (!isValidAnswer(answer)); 

         answers[i] = answer; 
      }

      //Process
      DriverExam exam = new DriverExam(answers); 

      //Results
      System.out.println("  RESULTS  "); 

      //Outputting total correct
      System.out.println("Total Correct: " + exam.totalCorrect()); 

      //Outputting total incorrect
      System.out.println("Total Incorrect: " + exam.totalIncorrect()); 

      String passed = exam.passed() ? "YES" : "NO"; 

      //Result pass or fail
      System.out.println("Passed: " + passed); 

      if (exam.totalIncorrect() > 0)
      {
          System.out.println("The incorrect answers are: "); 

          int missedIndex; 

          for (int i = 0; i < exam.totalIncorrect(); i++)
          {
            missedIndex = exam.questionsMissed()[i]+1; 
            System.out.print(" " + missedIndex); 
          }
      }
   } //end of main function

   //Returns true when answer is valid
   public static boolean isValidAnswer (String answer)
   {
      return "A".equalsIgnoreCase(answer) || 
         "B".equalsIgnoreCase(answer)
         || "C".equalsIgnoreCase(answer) || 
         "D".equalsIgnoreCase(answer); 
   }
} //end of Test class
\$\endgroup\$
  • \$\begingroup\$ How does a student miss a question if the only valid answers are A, B, C, and D? I'm assuming that is different from an incorrect answer, since two different terms are being used here. \$\endgroup\$ – h.j.k. Mar 30 '15 at 5:56
6
\$\begingroup\$

I would like to propose a number of refactorings:

I don't quite understand why the missed array is an int array which stores it's own indices if the answers are incorrect. A more logical choice would be a boolean array. We should also only calculate it only once, so we might as well do it in the constructer (which also only runs once per answer). The same applies to total number of correct/incorrect answers. The rest of my commentary is in the code.

// this should be static since it can be shared between all instances of the test
private static String[] correctAnswers = 
                                 {"B", "D", "A", "A", "C", "A", 
                                  "B", "A", "C", "D", 
                                  "B", "C", "D", "A", 
                                  "D", "C", "C", "B", "D", "A"};

// lets leave initializing to the constructor, and 
// let's store the values so we only calculate them once
// Also, make sure they are private to restrict access
private boolean[] missed;
private int correct;
private int incorrect;
private String[] userAnswers; 

//Process the user's answers
public DriverExam (String[] answers)
{
    missed = new boolean[answers.length];
    userAnswers = new String[answers.length];
    correct = 0;
    incorrect = 0; 

    for (int i = 0; i < answers.length; i++)
    {
        userAnswers[i] = answers[i]; 
        missed[i] = userAnswers[i].equalsIgnoreCase(correctAnswers[i])
        if (!missed[i]) {
            correct++;
        } else {
            incorrect++;
        }
    }
}

//Returns a boolean value if correct answers > 15 
public boolean passed()
{
    return correct >= 15; // don't use if/else when you are using a boolean expression
}

/*
 * Let's use the values we calculated to make the methods 
 * very simple and easy to read. In addition, we only calculate things once
 * which makes our code more efficient
 */

public int totalCorrect()
{
    return correct;
}

public int totalIncorrect()
{
    return incorrect;
}

public boolean[] questionsMissed()
{
    return missed;
}
\$\endgroup\$
  • \$\begingroup\$ +1 for great answer. I suggest adding a check that the passed answers array is the same size as the correctAnswers array, and throwing an appropriate exception (e.g. IllegalArgumentException) if not. \$\endgroup\$ – kiwiron Mar 29 '15 at 6:56
  • \$\begingroup\$ I think moving the initialization 'logic' into the constructor is a false assumption. After all: How often do the correct answers (and the number of answers change) Instead of using a boolean array, a List<Integer> would've been the waaay better choice to refactor to. \$\endgroup\$ – Vogel612 Mar 29 '15 at 7:52
  • \$\begingroup\$ why is List<Integer> better than boolean array? the missed array is used to store whether a question at an index was correct or not (2 choices = boolean), whereas Integer has billions, which is different. A list would indeed be a good next step but since the author seems to be only using arrays, I thought they might be beyond the scope of the question. \$\endgroup\$ – mleyfman Mar 29 '15 at 22:28
  • 1
    \$\begingroup\$ Thank you for a detailed response. I honestly have no idea why an int array is used, but it is part of the program's specifications and part of the instructions from my professor. \$\endgroup\$ – Duck Mar 30 '15 at 1:19
  • \$\begingroup\$ @Mary with that said, what @mleyfman said about 'calculate things once' is still applicable, unless an instance of DriverExam is mutable and the student's answers can be changed afterwards... \$\endgroup\$ – h.j.k. Mar 30 '15 at 5:48
1
\$\begingroup\$

From my comment:

How does a student miss a question if the only valid answers are A, B, C, and D? I'm assuming that is different from an incorrect answer, since two different terms are being used here.

I'm just going to assume that a missed answer can either be a null value or an empty String.

Also, are you expected not to use any of the Collections classes because of some reasons? I'm asking this as there seems to be a heavy reliance on arrays instead of the Collections framework...

Putting aside the user input validation parts, and focusing solely on your DriverExam class, I will too 'calculate things once' as suggested by mleyfman as such:

public class DriverExam {

    private static final String[] CORRECT_ANSWERS = { "B", "D", "A", "A", "C", "A", "B", "A", "C", "D", "B", "C", "D", "A", "D", "C", "C", "B", "D", "A" };
    private static final int EXPECTED_TOTAL = CORRECT_ANSWERS.length;
    private static final int PASSING_MARK = 15; // or would this be better expressed as 75%?

    private final int correct;
    private final int incorrect;
    private final boolean passed;
    private final int[] missed;


    public DriverExam(final String[] answers) {
        if (answers == null || answers.length != EXPECTED_TOTAL) {
            throw new IllegalArgumentException("Wrong answers specified.");
        }
        int correctCount = 0;
        final List<Integer> missedCounters = new ArrayList<>();
        for (int i = 0; i < EXPECTED_TOTAL; i++) {
            if (answers[i] == null || answers[i].isEmpty()) {
                missedCounters.add(Integer.valueOf(i));
            }
            if (answers[i].equalsIgnoreCase(CORRECT_ANSWERS[i])) {
                correctCount++;
            }
        }
        correct = correctCount;
        missed = new int[missedCounters.size()];
        int i = 0;
        for (final Integer value : missedCounters) {
            missed[i++] = value.intValue();
        }
        incorrect = EXPECTED_TOTAL - correctCount - missed.length;
        passed = correctCount > PASSING_MARK;
    }
}

As Java does not allow arrays of an undefined size, I have to decide between using a List to 'mark' missed answers, or resort to iterate answers twice - once to count the number of missed answers, and the second time to actually populate the missed[] array. I went with the former to make the implementation slightly easier.

After initializing the values correct, incorrect, passed and missed as such, the methods you are required to implement can simply return those values... I hope this helps you.

\$\endgroup\$
-6
\$\begingroup\$

The first thing I would do is convert A,B,C, and D to 1,2,3, and 4. Maybe 0-3, not sure yet. yeah 0,1,2,3. 0,1,2,3 just makes so much more sense than A,B,C,D. Very likely down the road it will prove to be a good move.

Do an ASCII to numeric conversion and subtract x41 or 65 if you are a normal human and think numerically in decimal.

The correct answers array is not going to cut it. It's actually in human alpha. Completely useless.

What we need is correct answers in a format that I can relate to. You might fall in place behind me and agree by the end of this. Maybe not, got to get there first.

Oops, this is a binary not decimal thing. 0,1,2,3 is not going to do.

Take the 0,1,2,3 and raise them to the power of 2.

0001
0010
0100
1000

That looks even better.

We could do this bitwise but that would require an if else. We do do not like if else. Intel microprocessors do not like if else either. Actually the reason I do not like if else is because processors do not like if else. In this business it's a good idea to be on the same side as the brain.

Let's separate the binary bits and create an array. Got to keep the micro happy.

0,0,0,1
0,0,1,0
0,1,0,0
1,0,0,0

No something is missing. What is it? Do you know? Nothing. This is perfect.

What do we have so far? Let's think about that.

User answers in the format of (and 50% less memory) 0,1,2,3 rather than A,B,C,D. Are we going to be graded on this?

It is important to keep keep memory structures small and aligned on address boundaries.

Intel 64 and IA-32 Architectures Optimization Reference Manual : 3.6.6 Data Layout Optimizations User/Source Coding Rule 6.(H impact, M generality) Pad data structures defined in the source code so that every data element is aligned to a natural operand size address boundary. If the operands are packed in a SIMD instruction, align to the packed element size (64-bit or 128-bit). Align data by providing padding inside structures and arrays. Programmers can reorganize structures and arrays to minimize the amount of memory wasted by padding.

3.6.4 Alignment Misaligned data access can incur significant performance penalties. This is particularly true for cache line splits. The size of a cache line is 64 bytes in the Pentium 4 and other recent Intel processors, including processors based on Intel Core microarchitecture. An access to data unaligned on 64-byte boundary leads to two memory accesses and requires several µops to be executed (instead of one). Accesses that span 64-byte boundaries are likely to incur a large performance penalty, the cost of each stall generally are greater on machines with longer pipelines.

OK, this is too much. We got correct answers and incorrect answers. Like I said, too much. One or the other, not both. which? We'll wing it for now.

We are told to create a new array for answers. Do we not already have one passed to us? Do we really want to waste electrons doing that? I don't but that's up to you.

for (int i = 0; i < Answers.length; i++){
  userAnswers[i] = Answers[i]; 
}

Never having written a line of java before this, I am not an authority on the syntax to say the least. But there should be an easier way to copy an array. Maybe like:

System.arraycopy( Answers, 0, userAnswers, 0, Answers.length);

How about kill two birds with one stone? Maybe more. More is better?

ASCII to numericord()

ord((userAnswers[i])

Mask off the lower case bit with 0xdf (0b11011111) to

convert lower to upper case.

ord((userAnswers[i]) & 0xdf))

And convert from ASCII ord() value to 0-3
subtract 0x41 to convert 'A' to zero, B to one, and etc.:

ord((userAnswers[i]) & 0xdf)) - 0x41)

Putting it all together:

for (int i = 0; i < userAnswers.length; i++){
  correctCount +=  correctAnswers[i][(ord((userAnswers[i]) & 0xdf)) - 0x41)] ;
}

Oh! Not much left to do! But the above is nearly the whole 9 yards.

Well if you haven't figured it out by now, here it the correctAnswer array converted to multi-dimensional array:

Where the correct alpha answer equates to the following:
A = [0] 1,0,0,0
B = [1] 0,1,0,0
C = [2] 0,0,1,0
D = [3] 0,0,0,1

correctAnswers =  new int[][]{{0,1,0,0},{0,0,0,1},{1,0,0,0},{1,0,0,0},{0,0,1,0},{1,0,0,0},{0,1,0,0},{1,0,0,0},{0,0,1,0},{0,0,0,1},{0,1,0,0},{0,0,1,0},{0,0,0,1},{1,0,0,0},{0,0,0,1},{0,0,1,0},{0,0,1,0},{0,1,0,0},{0,0,0,1},{1,0,0,0}};<br>


One line of code to get the number of correct answers:

for (int i = 0; i < Answers.length; i++){ correctCount +=  answers[i][(ord(userAnswers[i] & 0xdf) - 0x41)];}

Because the value of correctAnswers[i][(ord(userAnswers[i] & 0xdf) - 0x41)]; is either 1 for correct and zero for incorrect:
You can store the answers in a 2 dimensional array where correct answers are array[1] and incorrect answers are array[0]:

for (int i = 0; i < Answers.length; i++){
  answersScored[(correctAnswers[i][(ord(userAnswers[i] & 0xdf) - 0x41)]][i]= 1;
}

The above will give an array of both correct and incorrect answers.

Correct Answers answersCorrect:

System.arraycopy( answersScored[1], 0, answersCorrect, 0, answersScored[1].length);

And Incorrect answersIncorrect:

System.arraycopy( answersScored[0], 0, answersIncorrect, 0, answersScored[0].length);

What's the Point?

The use of arrays rather than if else is an unconventional programing methodology today. It is not widely known and because it is unconventional it is often Misunderstood.

The reason the array method is preferred over using an if else, is that if else structures should be avoided whenever possible. When looking at how the code executes in the microprocessor's execution engine. Intel has gone to extreme lengths to deal with branching and branch prediction.

Intel's recommendation in their 64 and IA-32 Architectures Optimization Reference Manual they make it very clear to remove branches whenever possible.

3.5.3 User/Source Coding Rule 4. (M impact, ML generality) Avoid the use of conditional branches inside loops

Intel's latest micro-architectures the have implemented Out of Sequence Instruction Execution where up to 8 instructions can be executed in one clock cycle. This technique does not work well with in conjunction with branch prediction.

While traditional programming uses an abundance of if else structures, it is not recommended in high performance designs.

In this DMV app it is not crucial, but when there are thousands of lines of code in a loop structure it is of paramount importance to use efficient coding techniques which have as few if else structures as possible.

\$\endgroup\$
  • 4
    \$\begingroup\$ There's no reason to smoosh a for loop down to a single line. Fewer lines doesn't mean better code. White space can be very valuable from a readability perspective. \$\endgroup\$ – nhgrif Mar 30 '15 at 1:31

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.