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.

I'm a beginner in Java so I would appreciate a review of following simple class - in fact it's my first real usage of enums.

The background: I'm parsing MySQL internal client-server protocol. One of the fields in server greeting and client authorization packet is "Client capabilities" which is 32 bit integer encoding 18 boolean flags ( if anyone is interested: http://forge.mysql.com/wiki/MySQL_Internals_ClientServer_Protocol#Handshake_Initialization_Packet)

I've read that the most 'java way' to implement such thing will be using EnumSet class.

I'll welcome any suggestions and other solutions - my needs are:

  1. I must be able to parse flags from 4 bytes big endian buffer
  2. To encode flags back to 4 bytes big endian buffer
  3. To check if some arbitrary flag is set

here's the code:

public class ClientCapabilities
{
    public enum Flag {
        CLIENT_LONG_PASSWORD (1<<0),
        CLIENT_FOUND_ROWS (1<<1),
        CLIENT_LONG_FLAG (1<<2),
        CLIENT_CONNECT_WITH_DB (1<<3),
        CLIENT_NO_SCHEMA (1<<4),
        CLIENT_COMPRESS (1<<5),
        CLIENT_ODBC (1<<6),
        CLIENT_LOCAL_FILES (1<<7),
        CLIENT_IGNORE_SPACE (1<<8),
        CLIENT_PROTOCOL_41 (1<<9),
        CLIENT_INTERACTIVE (1<<10),
        CLIENT_SSL (1<<11),
        CLIENT_IGNORE_SIGPIPE (1<<12),
        CLIENT_TRANSACTIONS (1<<13),
        CLIENT_RESERVED (1<<14),
        CLIENT_SECURE_CONNECTION (1<<15),
        CLIENT_MULTI_STATEMENTS (1<<16),
        CLIENT_MULTI_RESULTS (1<<17);

        public final int weight;

        Flag(Integer weight) {
            this.weight = weight;
        }
    }

    private final int originalInt;
    private final EnumSet<Flag> flags = EnumSet.noneOf(Flag.class);

    public ClientCapabilities(int capabilities)
    {
        originalInt = capabilities;
        for (Flag f : Flag.values()) {
            if ((capabilities & f.weight) > 0) {
                flags.add(f);
            }
        }
    }

    public int getAsInt()
    {
        return originalInt;
    }

    public EnumSet<Flag> getFlags() 
    {
        return flags;
    }

    @Override
    public String toString() {
        return flags.toString();
    }

    //only tests ahead - TestNG, not JUnit!
    @Test
    public static void testDecodingClientCapabilities()
    {
        byte[] exampleBuffer = new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 };
        EnumSet<Flag> expectedEnumSet = EnumSet.of(
                Flag.CLIENT_LONG_PASSWORD, 
                Flag.CLIENT_LONG_FLAG,
                Flag.CLIENT_LOCAL_FILES,
                Flag.CLIENT_PROTOCOL_41,
                Flag.CLIENT_INTERACTIVE,
                Flag.CLIENT_TRANSACTIONS,
                Flag.CLIENT_SECURE_CONNECTION,
                Flag.CLIENT_MULTI_STATEMENTS,
                Flag.CLIENT_MULTI_RESULTS
        );


        ClientCapabilities capabilities = new ClientCapabilities(
                PacketByteBuffer.readIntFromFourBytes(exampleBuffer,0));

        Assert.assertEquals(capabilities.getFlags(), expectedEnumSet);
    }

    @Test
    public static void testEncodingClientCapabilities()
    {
        byte[] exampleBuffer = new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 };
        byte[] targetBuffer = new byte[] { 0, 0, 0, 0, 0};

        ClientCapabilities capabilities = new ClientCapabilities(
                PacketByteBuffer.readIntFromFourBytes(exampleBuffer,0));

        PacketByteBuffer.writeIntToFourBytes(targetBuffer, 1, capabilities.originalInt);

        Assert.assertEquals(targetBuffer[1], (byte) 0x85);
        Assert.assertEquals(targetBuffer[2], (byte) 0xa6);
        Assert.assertEquals(targetBuffer[3], (byte) 0x03);
        Assert.assertEquals(targetBuffer[4], (byte) 0x00);
    }
}
share|improve this question

2 Answers 2

up vote 3 down vote accepted

It looks fine. Some notes about the tests:

Test methods should be in another class (ClientCapabilitiesTest) and in a separate source tree. For example, Maven projects usually store sources under src/main/java and test sources under src/test/java. (Introduction to the Standard Directory Layout).


The first parameter of assertEquals is the expected value and the second is the actual one, so you should change

assertEquals(targetBuffer[1], (byte) 0x85);

to

assertEquals((byte) 0x85, targetBuffer[1]);

JUnit error messages assumes this order.


Create more tests which helps defect localization. A parameterized test class would be fine:

import static org.junit.Assert.assertEquals;

import java.util.Arrays;
import java.util.Collection;
import java.util.EnumSet;

import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;

@RunWith(Parameterized.class)
public class ClientCapabilitiesTest {

    private final byte[] input;
    private final EnumSet<Flag> expected;

    @Parameters
    public static Collection<Object[]> data() {
        // TODO: add more cases
        final Object[][] data = new Object[][] {
                { new byte[] { (byte) 0x01, (byte) 0x00, (byte) 0x00, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD) },
                { new byte[] { (byte) 0x02, (byte) 0x00, (byte) 0x00, (byte) 0x00 }, 
                        EnumSet.of(Flag.CLIENT_FOUND_ROWS) },
                { new byte[] { (byte) 0x03, (byte) 0x00, (byte) 0x00, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD, Flag.CLIENT_FOUND_ROWS) },
                { new byte[] { (byte) 0x85, (byte) 0xa6, (byte) 0x03, (byte) 0x00 },
                        EnumSet.of(Flag.CLIENT_LONG_PASSWORD, 
                                Flag.CLIENT_LONG_FLAG, 
                                Flag.CLIENT_LOCAL_FILES,
                                Flag.CLIENT_PROTOCOL_41, 
                                Flag.CLIENT_INTERACTIVE, 
                                Flag.CLIENT_TRANSACTIONS,
                                Flag.CLIENT_SECURE_CONNECTION, 
                                Flag.CLIENT_MULTI_STATEMENTS, 
                                Flag.CLIENT_MULTI_RESULTS) } };
        return Arrays.asList(data);
    }

    public ClientCapabilitiesTest(final byte[] input, final EnumSet<Flag> expected) {
        this.input = input;
        this.expected = expected;
    }

    @Test
    public void testDecodingClientCapabilities() {
        final ClientCapabilities capabilities = new ClientCapabilities(
            PacketByteBuffer.readIntFromFourBytes(input, 0));

        Assert.assertEquals(expected, capabilities.getFlags());
    }
}

I'd add one case for every flag and some cases with multiple enabled flags.

share|improve this answer
    
Thanks for the answer, but I forgot to mention that tests are in TestNG (and it seems they have opposite expected-actual parameter order). –  sakfa Jan 8 '12 at 22:50

Looks fine to me. The only thing I have to notice is that if ((capabilities & f.weight) > 0) should instead be if ((capabilities & f.weight) != 0). The reason for this is that if you were using bit #31 (the 32nd bit) then the expression capabilities & f.weight would yield a negative number, so it would not pass the test for > 0.

share|improve this answer

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.