Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is an assignment question from school:

Write a method called licencePlate that takes an array of objectionable words and returns a random licence plate that does not have any of those words in it. Your plate should consist of four letters, a space, then three numbers.

import java.util.*;

class MethodAssign5{
static String licensePlate(String[] a){
    char[] lchars={'A','B','C','D','E','F','G','H','I','J','K','L','M','N','O','P','Q','R','S','T','U','V','W','X','Y','Z'};
    char[] nchars={'0','1','2','3','4','5','6','7','8','9'};
    boolean contain = true;
    char[] license = new char[8];
    license[4]=' ';
    Random generator = new Random();
    do{
        for(int i=0;i<4;i++){
            license[i]=lchars[generator.nextInt(26)];
        }
        for(int i=0;i<3;i++){
            license[i+5]=nchars[generator.nextInt(10)];
        }
        contain = false;
        for(String s:a){
            boolean same = true;
            char[] wchars = s.toCharArray();
            for(int i=0;i<4;i++){
                if(license[i]!=wchars[i]){
                    same = false;
                }
            }
            if(same==true){
                contain = false;
            }
        }
    }while(contain==true);
    String ans = new String(license);
    return ans;
}
public static void main(String[]args){
    String[] words ={"HAHA","GORD"};
    System.out.println(licensePlate(words));
}
}

As you can see, I have no idea if my solution is right. It's such a low probability.

What do you think about my solution? What should I do if this was an in-class test and had the same problem where the probability of getting the error is low?

share|improve this question
add comment

4 Answers

up vote 13 down vote accepted

A few suggestions, and then I'll get to how to test it:

  • Don't be afraid of whitespace -- your code is a little hard to read due to the lack of new lines. Also, it's fairly standard to indent method names inside of classes.
  • Always use descriptive names: String[] a tells me nothing about what that parameter is.
  • When x is a bool, x == true is equivalent to x.
    • On an extremely picky note, contains seems like a more natural variable name for contain (grammatically anyway)
  • It doesn't really matter when there's only 4 characters (so 4 * A.length runs), but when doing a linear comparison, you should typically bail out of it with break or continue
    • Also, you might want to look into indexOf, contains and equalsIgnoreCase
  • Obviously this is just a little homework assignment, so it would be a bit overkill, but I would put the license plate generation in its own class in a real application.
  • I would also pass the banned words as a parameter to the constructor instead of to a method -- that allows your instance to carry around the words without any farther down consumers having to know what the words are
    • Imagine that you make a machine that's used in the DMV. Imagine that this machine has a "generate license plate" button. When pressed, a screen simply shows the plate.
      • Passing the words to the method is like having to key in the banned words every time the button is pressed -- the DMV employee shouldn't be responsible for knowing/controlling those words
      • Storing the words inside of an instance (whether set with a setBannedWords method or passed to the constructor) is like configuring the machine at creation. The DMV employee no longer has to know what the words are, or be responsible for entering them. Years could go by before an employee realized "Oh my, I just noticed that the machine has never output a cuss word!"
      • For completeness: A third option would be to not filter the words -- it would then be up to the employee to go "Hrmmm, 'FART 743' is probably a bad license plate."

Ok, so does this work? Probably. It looks correct to my rusty-with-Java-eye anyway.

The easiest way to test it would probably be to whittle down the set of possible characters from A-Z to A-D and then make ABC a bad word. After a lot of test runs, you could be fairly certain that it was throwing out any ABCX or XABC.

This just made me realize: did your professor specify that the bad words would always be four characters? If not, your code is wrong.


So how could this be tested more properly-ish? Unfortunately it requires that the code become a lot longer. Your professor would probably think you got a bit carried away if you truly made this testable. Despite a fairly simple requirement, to test this with granularity would require breaking it down into quite a few pieces.

I can code this up if you want, but for the sake of brevity and time, I'll just describe it here.

The main problem with testing this is that all of the concerns are mixed together. You have a few concerns:

  • Generating a license plate
    • Generating the text part
    • Generating the number part
  • Filtering a license plate

Note that separating this concerns allows you to test them independently.

"Does my class properly generate and filter license plates?" becomes "Does class X properly generate license plates? Does class Y properly filter license plates?"

The second option is easier to test because it allows you to pass in a license plate generator that is rigged to generate bad words.

The text and number generation should arguably be separated. That would allow for easily reusing generated numbers when a bad word is in the word part, but other than that, the added complexity would probably not be worth it.

One option would be to use an interface:

public interface LicensePlateGenerator {
    public String generateLicensePlate();
}

You could then have your classes implement it:

public class LicensePlateGeneratorRandom implements LicensePlateGenerator {

    private final lchar[] = {'A', 'B', 'C', ...};

    private final nchar[] = {'0', '1', '2', ...};

    public String generateLicensePlate() {

        //Randomly grab 4 lchars
        //Randomly grab 3 nchars            

    }

}

public class LicensePlateGeneratorFiltered implements LicensePlateGenerator {

    private final LicensePlateGenerator gen;
    private final List<String> badWords;

    public LicensePlateGeneratorFiltered(LicensePlateGenerator generator, List<String> badWords) {

        this.gen = generator;
        this.badWords = badWords;

    }

    public String generateLicensePlate() {

        //It might be a good idea to test how many plates have already been generated
        //An infinite loop of generating could happen depending on the underlying generator
        //and what kind of badWords are defined

        String lp;

        do {
            lp = gen.generateLicensePlate();
        } while (isBad(lp));

        return lp;

    }

    private boolean isBad(String lp) {
        //Return false if lp contains any badWords
        //and true otherwise
    }

}

Note how this is transparent to any consuming class. A consuming class doesn't need to know whether it has a LicensePlateGeneratorRandom or LicensePlateGeneratorFiltered; it just needs to know it has a LicensePlateGenerator.

Note that this allows you to test very easily. Testing your random generator means just checking a few outputs. Checking your filter could be done by rigging a generator:

(I would probably define this as an anonymous class in actual testing code, but my Java is too rusty to remember the syntax for that :p)

public class LicensePlateGeneratorFake implements LicensePlateGenerator {

    private final List<String> words;
    private Iterator<String> currentPos;

    public LicensePlateGeneratorFake(List<String> words) {
        this.words = words;
        currentPos = this.words.iterator();
    }

    public function generateLicensePlate() {
        if (!currentPos.hasNext()) {
            //Something should happen here...  
            //could always just loop back around, or it
            //might also be worth considering having the interface
            //declare a certain exception as being possible in this method.  
            //(That would also allow a way out of the infinite loop in the
            //(filter generator)
            //throw new LicensePlateGeneratorException("...");
            //(that could be the base class, and then a LicensePlateGeneratorInfiniteLoopException
            //could be thrown in the filter class)
        }
        //Obviously the end of the iterator should be checked, but I'm lazy
        return currentPos.next() + " 123";
    }

}

You then configure this fake generator to generate a certain stream of words ("ABCD", "DCBA", etc). Then, you pass this fake generator to the filter, and give the filter a list of words you know that the fake generator will generate.

If you tell the fake generator to generate "ABCD" and you tell the filter to reject "ABCD", you can then test success by whether or not the filter generator returns "ABCD" in any of the generations.


An alternative approach would be to have the filter not be a generator. Instead, your consuming code would be responsible for configuring its own filters and then generating plates until a suitable one is found.

Both designs have fairly strong pros and cons.


(Note: I completely bastardized the class names in my examples. In a real application, namespaces should be used. The code formatting on here makes namespaces a bit clumsy though, so I didn't use them.)

share|improve this answer
1  
Holy, it took me so long to read all of your writings... I thank you very much for your efforts. By the way, for my question bad words would always be four characters. This is grade 12 computer science, that's why rules are simple :) –  cook cook Oct 19 '12 at 1:46
1  
Crikey thats comprehensive :) –  dreza Oct 19 '12 at 7:57
1  
+42 great, comprehensive answer. –  David Harkness Oct 20 '12 at 4:42
add comment

Some small notes (actually the last few are more or less small comments to Corbin's great answer):

  1. The licensePlate method should validate its input parameters. Does it make sense to call it with null? If not, check it and throw a NullPointerException. (Effective Java, Second Edition, Item 38: Check parameters for validity)

  2. Method names should be verbs, for example, licensePlate should be generateLicensePlate, while class names shouldn't be verbs. (Check Code Conventions for the Java Programming Language, Naming Conventions and Clean Code, page 25.)

  3. The code contains lots of magic numbers. Using named constants instead of numbers would make the code more readable and less fragile. If you have to modify their value (for example, you need three-letter, three-digit plates instead of four-letter, three-digit plates) it's easy to forget to do it everywhere.

  4. An endless while loop with a return seems more natural and easier to read to me than the do-while loop:

    while (true) {
        final String licensePlate = generator.generateLicensePlate();
        if (!isBad(licensePlate)) {
            return licensePlate;
        }
    }
    
  5. For the reasons above I'd prefix the classes: RandomLicensePlateGenerator, FilteredLicensePlateGenerator etc.

  6. The LicensePlateGeneratorFake class could be omitted if you use a mock library like EasyMock.

share|improve this answer
1  
The assignment requires the name of the method to be licensePlate. Why professors think teaching their students bad style is beyond me. –  Eva Jan 17 '13 at 2:34
add comment

While Corbin's answer is great (and I'd cetainly recommend accepting it over this), it buries the key point: your method does two things. This makes testing, understanding, and maintaince harder.

You are supposed to a) generate a licence number, and b) filter out unacceptable ones.

This should properly be split into at least 3 function, 1 to generate, 1 to reject, and 1 that calls the other 2 and only returns the result of the first if not rejected.

At that point you should see what needs testing, what can be tested, and how to do your test.

As for your final question: you fiddle with the process to make the error both frequent and repeatable.

share|improve this answer
add comment

A few small additions:

  1. The Random class is generally made a static field to avoid creating it multiple times.
  2. The letter and number character arrays can be created using a simple method

    private char[] getCharsInRange(final char lowerBound, final int size) {
        final char[] result = new char[size];
    
        for (int i = 0; i < size; ++i) {
            result[i] = (char)(lowerBound + i);
        }
    
        return result;
    }
    

    Names should be better, but I'm feeling lazy.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.