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.

My first test (randomUpperCaseLetter) checks if the returned random letter is an uppercase ASCII one. The second one (compareCharactersWithAsciiCodes) is just me trying to understand Java.

package com.company.letter;

import com.company.RandomLetter;

import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class RandomLetterTest {
    @Test
    public void randomUpperCaseLetter() {
        char letter = new RandomLetter().generate();
        int upperCaseAAsciiCode = 65;
        int upperCaseZAsciiCode = 90;
        assertTrue(letter >= upperCaseAAsciiCode && letter <= upperCaseZAsciiCode);
    }

    @Test
    public void compareCharactersWithAsciiCodes() {
        assertEquals(65, 'A');
        assertEquals(90, 'Z');
        assertEquals(76, 'L');
        assertEquals(77, 'M');
        assertEquals(78, 'N');
    }
}

Is the first test valuable enough to keep it? What about the second one? Uncle Bob say something about tests to understand how the language works. I don't remember when/where.

Do you have suggestions to improve its readability?

share|improve this question
    
It would be a tiny bit clearer to express uppercase a and z as constants: private final static int UPPERCASE_A = 65; private final static int UPPERCASE_Z = 90; –  Carl Manaster May 7 at 1:17
    
Once you have answers, please don't edit the code any more. meta.codereview.stackexchange.com/questions/1763/… I've reverted your last change. –  Marc-Andre May 8 at 13:01

3 Answers 3

The main thing I've noticed is the usage of int type to represent characters. Characters are stored in the memory as numbers but as for testing I would expect to see char in its human readable form. For example:

public void randomUpperCaseLetter() {
    char letter = new RandomLetter().generate();
    int upperCaseAAsciiCode = 65;
    int upperCaseZAsciiCode = 90;
    assertTrue(letter >= upperCaseAAsciiCode && letter <= upperCaseZAsciiCode);
}

should be written for clarity as

public void randomUpperCaseLetter() {
    char letter = new RandomLetter().generate();
    assertTrue(letter >= 'A' && letter <= 'Z' );
}

To better test the generating letter functionality, I would call generate method multiple times (possibly in a loop) to generate more letters.

For the second test method, it seems to be unnecessary. The letter 'A' will always have ASCII character code 65 and there is no need to test it.

public void compareCharactersWithAsciiCodes() {
    assertEquals(65, 'A');
    :
}
share|improve this answer
    
I really like your suggestion Max Zoom! Thanks! The second test is a "learning/remembering a programming language feature" test. I don't have references about it and I don't even know if this kind of test has a name :) –  Thom Thom Thom May 7 at 22:54
    
Sorry, fixed Max Zoom. :) –  Thom Thom Thom May 7 at 23:11
    
Thanks! Most people does not like to hear about their code, but I think it is a way to learn new things. –  MaxZoom May 7 at 23:13
    
Of course it is. But it is even better when you are far from the person talking about your bad code :P –  Thom Thom Thom May 7 at 23:16

randomUpperCaseLetter()

I will assume that RandomLetter.generate() actually does return something randomly* generated. (Maybe it should be named generateUpperCase() instead?)

Unit tests should be deterministic. Without changing your code they should run the same every time. Let's say you run this test and it returns a valid 'A' and you think it works, well, what if it's possible it could also return 'a'? So you set up a loop to run it a few times, say 100.

@Test
public void randomUpperCaseLetter() {
    int upperCaseAAsciiCode = 65;
    int upperCaseZAsciiCode = 90;
    for (int i = 0; i < 100; i++) {
        char letter = RandomLetter.generate();
        assertTrue(letter >= upperCaseAAsciiCode && letter <= upperCaseZAsciiCode);
    }
}

If the code is actually random* then you can't be certain it is correct like this. Yes, you can be more certain like this, but it's still not really perfect. Even if you do it until it gives you every upper case letter, you can't do it until it doesn't give you every other character, that is a paradox.

Edit: Here is an answer on the programming stackexchange site with a lot of goo information on unit testing randomness.

All that said, in practice I think it's better than nothing to have something like this and have it run a high number of times (keeping it under a second or so execution time) to be more certain but it just doesn't feel right.

*: Pseudo-random, whatever, that's out of the scope of the question I think.

compareCharactersWithAsciiCodes()

There's nothing wrong with making snippets like this to learn something in the same way there's nothing wrong with making a main method in a new Example class to try something, but it's not the sort of thing I would check in to a repository.

This isn't checking your code for correct behavior, it's checking that the JVM's specification is correct, which isn't the point of unit testing. It's safe to assume (in a unit test) that the specification of the language works as intended.

share|improve this answer
    
generateUpperCase seems to be clear enough. A don't like the loop idea but I don't find a better solution. Please see my update. Thanks! –  Thom Thom Thom May 7 at 23:07
    
Yeah, the loop is ugly, but that's sort of the nature of the beast when trying to figure out a way to unit test something random. If the point of the method was to give a random uppercase letter without replacement (If it gives 'A' then the next could only be 'B' - 'Z') it'd be a lot easier, because then you can easily say that after 26 runs it should be returning null (or throwing errors depending on implementation). –  Captain Man May 8 at 12:56

Without having access to the class RandomLetter let's see what we can deduce from your tests.

@Test
public void randomUpperCaseLetter() {
    char letter = new RandomLetter().generate();
    int upperCaseAAsciiCode = 65;
    int upperCaseZAsciiCode = 90;
    assertTrue(letter >= upperCaseAAsciiCode && letter <= upperCaseZAsciiCode);
}

The name of the test looks good to me; we sure do test that an uppercase letter has been generated. The thing is we don't test if it's actually random. You're producing only one letter, checking if it's in the range of uppercase letters, and then be done with it. We've never actually checked that the letters that are produced are different/random every time. How can you be sure that it's random?

@Test
public void compareCharactersWithAsciiCodes() {
    assertEquals(65, 'A');
    assertEquals(90, 'Z');
    assertEquals(76, 'L');
    assertEquals(77, 'M');
    assertEquals(78, 'N');
}

The name is still good to me, we are indeed comparing char with ASCII codes. The thing is, it's not testing anything related to RandomLetter. It's just testing if in Java the char A is equal to 65; and unless the language changes, your test is not really useful to test the class.

So what can you do? First, I'm glad you have unit tests! This is one of the things I would like to see more often when someone is learning a language. Second, try to cover every bit of your code by asking (but not limited to):

  • Did you test all the if else of your code?
  • Did you try to test that generate() gives some different letters each time?
  • Did you test all the public methods ? Did you test some corner/edge cases of your code?

There are tools to help you see what is the coverage of your tests, but you can begin with manually checking what your tests cover.

share|improve this answer
1  
Obligatory xkcd: xkcd.com/221 –  h.j.k. May 7 at 3:10
    
Funny xkcb, h.j.k.! Hahahaha –  Thom Thom Thom May 7 at 22:59
    
Marc-Andre, I've updated my question with the RandomLetter code. I don't know how to test its randomness. Do you have any example? Actually I don't have any if/else. Thanks! –  Thom Thom Thom May 7 at 23:03

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.