CODEWITHSUNDEEP
java
Drathier
Drathier

Reputation: 14519

Java static variables, what am I doing wrong?

I'm trying to make sure no two instances of a class have the same priority/id value. I'm currently trying to save booleans for each priority value, but exceptions are never thrown in my unit tests. Why doesn't this code work?

public abstract class AbstractCard implements Card {
    private int priority;
    protected Robot robot;

    private static boolean[] prioritiesUsed = new boolean[1000]; // defaults to false

    ...[omitted code]

protected AbstractCard(int lowPriority, int highPriority) {

        // TODO: this doesn't work!
        // different instances can have the same priority right now
        // that shouldn't be allowed, so something's broken somewhere here!
        // this code will still work as good as the placeholder code we had, so I'm pushing this broken code

        Random random = new Random();

        // try a few times to get a random priority that hasn't been used before
        for (int tries = 0; tries < 10; tries++) {
            // get random priority p in range low <= p <= high
            this.priority = random.nextInt((highPriority - lowPriority) + 1) + lowPriority;

            // is this priority already taken?
            if (prioritiesUsed[this.priority] == false) {
                prioritiesUsed[this.priority] = true;
                return;
            }
        }

        // screw randomizing, it didn't work. Let's just increment untill we find a free one.
        int infiniteLoopDetector = this.priority - 1;
        while (prioritiesUsed[this.priority]) {
            this.priority++;

            if (this.priority > highPriority) { // about to leave range, wrap around and try from lower bound
                this.priority = lowPriority;
            }

            if (this.priority == infiniteLoopDetector) {
                throw new DeckPriorityIntervalFilledException(
                    String.format("All priority values between %d and %d are already taken by other cards",
                            lowPriority, highPriority));
            }
        }

    }

Here's the unit test, using groovy in intellij

note: class MoveCard extends AbstractCard

void testConstructor() {
        // try to create 101 cards inside a 100 card interval
        try {
            for (int i = 0; i < 100; i++) {
                new MoveCard(42, 141); // 42 <= priority <= 141
            }
        } catch (RuntimeException e) {
            fail("exception thrown: " + e.toString());
        }

        // and add the 101st one, that should throw an exception
        try {
            AbstractCard ac = new MoveCard(42, 141);
            println(ac.priority);
            ac = new MoveCard(42, 141);
            println(ac.priority);
            ac = new MoveCard(42, 141);
            println(ac.priority);
            ac = new MoveCard(42, 141);
            println(ac.priority);
            ac = new MoveCard(42, 141);
            println(ac.priority);


            fail("exception wasn't thrown!");
        } catch (RuntimeException e) {
            // all ok
        }
    }

Upvotes: 0

Views: 108

Answers (1)

Marco13
Marco13

Reputation: 54639

You are simply not setting

prioritiesUsed[this.priority] = true;

after trying out the priorities with the while-loop.

General hints:

During debugging, I'd strongly recommend to use a common Random instance that is initialized with a particular random seed. This instance will always provide the same sequence of random numbers, making the runs reproducable. So replace your local Random instance with a 

private static Random random = new Random(0);

However, your approach is ... not very elegant. As others have pointed out, managing the priorities with a Set would be much easier. As suggested, you could use a factory to encapsulate the management of the priorities, but the code could roughly look like this:

abstract class AbstractCard
{
    int priority;
    private static Set<Integer> usedPriorities = new LinkedHashSet<Integer>();
    private static Random random = new Random(0);

    protected AbstractCard(int lowPriority, int highPriority)
    {
        Set<Integer> availablePriorities = new LinkedHashSet<Integer>();
        for (int i = lowPriority; i <= highPriority; i++)
        {
            availablePriorities.add(i);
        }
        availablePriorities.removeAll(usedPriorities);
        if (availablePriorities.size() == 0)
        {
            throw new RuntimeException(String.format(
                "All priority values between %d and %d "
                    + "are already taken by other cards", lowPriority,
                highPriority));
        }
        List<Integer> priorities = new ArrayList<Integer>(availablePriorities);
        int index = random.nextInt(priorities.size());
        this.priority = priorities.get(index);
        usedPriorities.add(this.priority);
    }

}

Upvotes: 1

Related Questions