19
\$\begingroup\$

Play my game!

Having the name that I do (Simon), of course I have to participate in this challenge! (Even though I am a bit "late", but Simon says "have forgiveness!")

This is an implementation of the Simon Says challenge using libGDX. I started using this framework a couple of months ago, and I love the fact that it can create games for Desktop, Android, iOS and HTML5, at once.

As it is libGDX, and Android support is needed, I'm limited to Java 6. As I'm also using GWT, there's some restriction there as well.

Class Summary

I have not included the several different launchers here, as I am just using the defaults that comes with a LibGDX project there.

  • SimonGame: Main LibGDX entry point
  • MenuScreen: Shows the different implemented playing "difficulties"
  • GameScreen: The main game screen, with the buttons you are supposed to remember the sequence of and click at
  • SequenceGenerator: Interface for generating a sequence
  • AppendingGenerator: Adds one new random button each time, but keeping the old sequence
  • SimpleGenerator: Completely generates a new sequence each time

Dependencies

  • LibGDX
  • GWT

Code

This code can also be downloaded from GitHub

AppendingGenerator.java:

public class AppendingGenerator implements SequenceGenerator {
    private final Random random = new Random();
    private int count;
    private List<Integer> recent;

    public AppendingGenerator(int count) {
        this.count = count;
        this.recent = new ArrayList<Integer>();
    }

    @Override
    public void generate(int numButtons, List<Integer> result) {
        while (recent.size() < count) {
            recent.add(random.nextInt(numButtons));
        }
        result.addAll(recent);
        this.count++;
    }
}

GameScreen.java:

public class GameScreen implements Screen {
    private final SimonGame game;
    private final Button[] images;
    private final LinkedList<Integer> sequence = new LinkedList<Integer>(); // GWT does not support 'Deque' interface
    private final Table table = new Table();
    private final SequenceGenerator generator;
    private final int numButtons;
    private static final float fadeTime = 0.7f;

    public GameScreen(SimonGame game, int numButtons, SequenceGenerator generator) {
        this.game = game;
        this.generator = generator;
        this.numButtons = numButtons;
        this.images = new Button[numButtons];
        table.setFillParent(true);
        for (int i = 0; i < images.length; i++) {
            final int index = i;
            images[i] = new TextButton("", game.getSkin());
            images[i].addListener(new ClickListener() {
                @Override
                public void clicked(InputEvent event, float x, float y) {
                    buttonClick(index);
                }
            });
            table.add(images[i]).expand().fill();
            if (i % 2 == 1) {
                table.row();
            }
        }
        generateSequence();
    }

    private void generateSequence() {
        sequence.clear();
        generator.generate(numButtons, sequence);
        showSequence();
    }

    private void showSequence() {
        setButtonsEnabled(false);
        showSequence(0);
    }

    private void setButtonsEnabled(boolean enabled) {
        for (Button image : images) {
            image.setTouchable(enabled ? Touchable.enabled : Touchable.disabled);
        }
    }

    private void showSequence(final int index) {
        if (sequence.size() <= index) {
            setButtonsEnabled(true);
            return;
        }
        Integer i = sequence.get(index);
        Actor actor = images[i];
        actor.addAction(sequence(alpha(0.7f, fadeTime), alpha(1.0f, fadeTime), run(new Runnable() {
            @Override
            public void run() {
                showSequence(index + 1);
            }
        })));
    }

    private void buttonClick(int index) {
        if (sequence.getFirst() == index) {
            sequence.removeFirst();
            if (sequence.isEmpty()) {
                generateSequence();
            }
        }
        else {
            game.setScreen(new MenuScreen(game));
        }
    }

    @Override
    public void render(float delta) {

    }

    @Override
    public void resize(int width, int height) {

    }

    @Override
    public void show() {
        game.getStage().addActor(table);
    }

    @Override
    public void hide() {
        table.remove();
    }

    @Override
    public void pause() {

    }

    @Override
    public void resume() {

    }

    @Override
    public void dispose() {

    }
}

MenuScreen.java:

public class MenuScreen implements Screen {

    private static final int NUM_BUTTONS = 4;
    private final SimonGame game;
    private final Table table;

    public MenuScreen(final SimonGame game) {
        this.game = game;
        this.table = new Table();

        table.setFillParent(true);
        addPlayButton("Simple", NUM_BUTTONS, new AppendingGenerator(3));
        addPlayButton("Random", NUM_BUTTONS, new SimpleGenerator(3));
    }

    private void addPlayButton(String text, final int numButtons, final SequenceGenerator generator) {
        TextButton playButton = new TextButton(text, game.getSkin());
        playButton.addListener(new ClickListener() {
            @Override
            public void clicked(InputEvent event, float x, float y) {
                game.setScreen(new GameScreen(game, numButtons, generator));
            }
        });
        table.add(playButton).expand().fill();
    }

    @Override
    public void render(float delta) {

    }

    @Override
    public void resize(int width, int height) {

    }

    @Override
    public void show() {
        game.getStage().addActor(table);
    }

    @Override
    public void hide() {
        table.remove();
    }

    @Override
    public void pause() {

    }

    @Override
    public void resume() {

    }

    @Override
    public void dispose() {

    }
}

SequenceGenerator.java:

public interface SequenceGenerator {
    void generate(int numButtons, List<Integer> result);
}

SimonGame.java:

public class SimonGame extends Game {
    private SpriteBatch batch;
    private Camera camera;
    private Stage stage;
    private Skin skin;

    @Override
    public void create () {
        batch = new SpriteBatch();
        camera = new OrthographicCamera(800, 480);
        stage = new Stage(new FitViewport(800, 480, camera), batch);
        skin = new Skin(Gdx.files.internal("data/uiskin.json"));
        setScreen(new MenuScreen(this));
        Gdx.input.setInputProcessor(stage);
    }

    @Override
    public void render () {
        Gdx.gl.glClearColor(1, 0, 0, 1);
        Gdx.gl.glClear(GL20.GL_COLOR_BUFFER_BIT);
        batch.setProjectionMatrix(camera.combined);
        batch.begin();
        super.render();
        batch.end();
        stage.act(Math.min(Gdx.graphics.getDeltaTime(), 1 / 30f));
        stage.draw();
    }

    public Stage getStage() {
        return stage;
    }

    public Skin getSkin() {
        return skin;
    }
}

SimpleGenerator.java:

public class SimpleGenerator implements SequenceGenerator {
    private final Random random;
    private int count;

    public SimpleGenerator(int count) {
        this.count = count;
        this.random = new Random();
    }

    @Override
    public void generate(int numButtons, List<Integer> result) {
        for (int i = 0; i < count; i++) {
            result.add(random.nextInt(numButtons));
        }
        count++;
    }
}

Usage / Test

Available to play here!

Questions

My primary concern is with passing on the SimonGame to the screens. I am perfectly aware that I can extract an interface, but is it really worth it? The screens would not be useful without this game, and the game would not be useful without those screens - is it worth decoupling them? It seems to be very common for LibGDX projects to pass their main game class on to other classes - many other projects even use the camera, stage, etc as package-private fields which I wanted to avoid, I think I prefer a getter instead.

My secondary concern is the data structure for the sequence. I don't think there's too much wrong with using a LinkedList here. And I still want to support different sequence generators.

My third concern is everything else. Comment on what you want.

Note that I am fully aware that the "graphical design" sucks. Luckily, this is Code Review and not Graphics Review

\$\endgroup\$
12
\$\begingroup\$

The only note I can add to this mostly A+ code is that you should generally avoid using Collections for boxed primitives (e.g. LinkedList<Integer> etc.) - that's a rule of thumb; although it doesn't matter much in such a small application, with bigger code and 1k+ elements per collection - and especially on Dalvik - you'll run into trouble with it, sooner or later. If a simple array is not an option (e.g. you want the collection to be dynamic or use it as deque or whatever) there are some classes, both libGDX-based and e.g. Apache Commons or even Guava ones that allow using Collection-type abstraction on primitives with native speed.

I'd suggest taking a look at https://github.com/libgdx/libgdx/tree/master/gdx/src/com/badlogic/gdx/utils - e.g. IntArray and IntSet might be good ideas in many cases. OTOH, those libGDX classes lack Java Collections-compliant interfaces (mostly because it's basically incompatible with primitive-based classes), see https://github.com/libgdx/libgdx/issues/2359 and, for a possible solution, here: https://github.com/libgdx/libgdx/commit/9ff7125482cb822034acdb1e3ebfe5af624d9433 (view wrappers allowing both direct (unboxed) and collection-like (boxed) access) - note it's still waiting to get into trunk, sigh.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is really my favorite answer so far! You're right, there are some LibGDX specific answers I did not consider. \$\endgroup\$ – Simon Forsberg Dec 16 '14 at 22:18
6
\$\begingroup\$
public AppendingGenerator(int count) {
    this.count = count;
    this.recent = new ArrayList<Integer>();
}

The count parameter might be better off as initialCount - otherwise it's not immediately crystal-clear why one would want to pass an int to that constructor. Also, recent is more of less of a little lie, since it's actually keeping the whole sequence - I think sequence would make a better name here, especially given the name of the base class you're implementing.

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

It's not so great that GameScreen is in charge of both showing the sequence and validating the user clicks. The only common element in these operations is the sequence. And these operations use the sequence very differently. One just reads from it, the other pops off elements from the front.

Notice how the showSequence and buttonClick methods are sort of logically orthogonal. They are completely unrelated, and it would be fatal if they got called in the wrong order. Of course, the rest of the class ensures that they don't get called in the wrong order, but the overall logic is not very clear.

You would have clearer logical flow if you split this class into two (or more), and converge to each class having a single responsibility. The current class has too many responsibilities, and its methods lack strong cohesion. The class decomposition and the overall logic is a bit confusing to me.

The recursive showSequence approach is a bit smelly too (though it would be overly paranoid to fear for a stack oveflow), as well as the LinkedList used for both sequential traversal and random access.


I don't think there's too much wrong with using a LinkedList here.

I guess you chose LinkedList so that you can pop off items from the front easily. But that's only half of the story here. In the recursive showSequence calls, you're accessing the elements one by one, but not in sequence, but as independent random accesses, which gets very inefficient as the sequence gets longer during the game.

I suggest to change the type to a List, and rework buttonClick to use an index variable to keep track of the current position, instead of popping off elements. There is no real downside to this refactor, it won't be particularly ugly, and it will eliminate the inefficiency in showSequence. As a result, the reason for using a LinkedList will vanish, along with your dilemmas about it.


This is a void method with practically an out-parameter:

@Override
public void generate(int numButtons, List<Integer> result) {
    while (recent.size() < count) {
        recent.add(random.nextInt(numButtons));
    }
    result.addAll(recent);
    this.count++;
}

You can rewrite this to return recent, and use addAll in the caller. The result will be cleaner, for example clean up the poorly named result variable.


The magic numbers 0.7f and 1.0f in alpha(0.7f, fadeTime), alpha(1.0f, fadeTime) would be good to move to constants.


800 and 480 just happen to be the same on these lines or should they be the same?

    camera = new OrthographicCamera(800, 480);
    stage = new Stage(new FitViewport(800, 480, camera), batch);

Either way, they would be better as constants near the top of the file.

\$\endgroup\$
  • \$\begingroup\$ I did consider returning a List but that would require many of the sequence generators to instantiate a new list inside the method. I did think of a couple of alternative method headers for that, couldn't really decide which one to use :) \$\endgroup\$ – Simon Forsberg Dec 16 '14 at 21:22
  • 3
    \$\begingroup\$ 1.0f is as much of a magic number as 100 is a magic number \$\endgroup\$ – Simon Forsberg Dec 16 '14 at 21:22
  • \$\begingroup\$ I didn't look too close, but it would seem the 0.7 and 1.0 are related somehow, so if you can give a meaningful name to 0.7 then I'd guess you can do likewise for 1.0. Or would ONE really be the best name for 1.0 ? \$\endgroup\$ – janos Dec 16 '14 at 21:31
  • 3
    \$\begingroup\$ It's the alpha values to fade to, 1.0f being FULL_ALPHA, but that is so commonly known to be exactly 1.0 I don't think it would be a good idea to extract that to a constant. \$\endgroup\$ – Simon Forsberg Dec 16 '14 at 21:33
  • \$\begingroup\$ All your comments are about 1.0, and conveniently sidestep 0.7, the one that's harder to defend. If you can find a good name for 0.7, then the name for 1.0 might come naturally. If you can't, you can't, it's just a thought. \$\endgroup\$ – janos Jan 20 '15 at 23:23

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.