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.

Description

This is the good old game Memory with a twist: Every time you pick a wrong pair, the two tiles you chose will switch their location. So sometimes you might think that a tile is at one location when in fact... it has moved. And it might feel like you have no idea where it is anymore.

Author of this game is not responsible for any broken keyboards, screens and/or mouse devices.

I am using a semi-Java8-compliant version of GWT. It does not support the Stream API.

Where to play?

The game can be played here: http://www.zomis.net/codereview/memory/MemoryGWT.html

Class Summary

  • MemoryGWT.java, MemoryGWT.html: Main GWT Entry Point.
  • MemoryGWT.css: Just some simple CSS.
  • MemoryBoard.java: The view class used for the main Memory Board
  • FieldView.java: The view for each tile.
  • ListUtils.java: Excluded from the review as it is not my code originally. Simply contains an implementation of shuffle as Collections.shuffle does not work in GWT.

Code

FieldView.java: (39 lines, 760 bytes)

public class FieldView implements IsWidget {

    private static final String HIDDEN_LABEL = "";
    private final Button widget;
    private int value;

    public FieldView(int value) {
        this.value = value;
        widget = new Button(HIDDEN_LABEL);
        widget.setStyleName("game-button", true);
    }

    @Override
    public Button asWidget() {
        return widget;
    }

    public int getValue() {
        return value;
    }

    public void setValue(int value) {
        this.value = value;
    }

    public void showValue() {
        widget.setText(String.valueOf(value));
    }

    public void hideValue() {
        widget.setText(HIDDEN_LABEL);
    }

}

MemoryBoard.java: (86 lines, 2081 bytes)

public class MemoryBoard implements IsWidget {

    private final Grid grid;
    private final Random random = new Random();
    private FieldView previousClicked;
    private boolean timerRunning;

    public MemoryBoard(int width, int height) {
        if ((width * height) % 2 != 0) {
            throw new IllegalArgumentException("width * height must be an even number");
        }
        grid = new Grid(height, width);
        grid.setStyleName("game");
        List<Integer> ints = new ArrayList<>();
        for (int i = 0; i < width * height / 2; i++) {
            ints.add(i);
            ints.add(i);
        }
        ListUtils.shuffle(ints, random);

        for (int x = 0; x < width; x++) {
            for (int y = 0; y < height; y++) {
                int value = ints.remove(ints.size() - 1);
                FieldView view = new FieldView(value);
                view.asWidget().addClickHandler(e -> clicked(view));
                grid.setWidget(y, x, view);
            }
        }

    }

    private void clicked(FieldView view) {
        if (view == previousClicked) {
            return;
        }
        if (timerRunning) {
            return;
        }
        if (previousClicked != null) {
            boolean same = previousClicked.getValue() == view.getValue();
            view.showValue();
            if (!same) {
                Timer timer = new Timer() {
                    @Override
                    public void run() {
                        // switch the two values
                        int previous = previousClicked.getValue();
                        previousClicked.setValue(view.getValue());
                        view.setValue(previous);
                        view.hideValue();
                        previousClicked.hideValue();
                        previousClicked = null;
                        timerRunning = false;
                    }
                };
                timerRunning = true;
                timer.schedule(2000);
            }
            else {
                previousClicked = null;
            }
        }
        else {
            view.showValue();
            previousClicked = view;
        }

    }

    @Override
    public Widget asWidget() {
        return grid;
    }

}

MemoryGWT.java: (15 lines, 345 bytes)

public class MemoryGWT implements EntryPoint {

    @Override
    public void onModuleLoad() {
        final MemoryBoard memory = new MemoryBoard(6, 6);

        RootPanel.get("gameContainer").add(memory);

    }
}

MemoryGWT.css

.game-button {
    width: 42px;
    height: 42px;
}

MemoryGWT.html

<!doctype html>
<!-- The DOCTYPE declaration above will set the     -->
<!-- browser's rendering engine into                -->
<!-- "Standards Mode". Replacing this declaration   -->
<!-- with a "Quirks Mode" doctype is not supported. -->

<html>
  <head>
    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
    <link type="text/css" rel="stylesheet" href="MemoryGWT.css">
    <title>Memory Extreme</title>

    <script type="text/javascript" src="memorygwt/memorygwt.nocache.js"></script>
  </head>

  <body>

    <h1>Memory</h1>
    <p>Good old memory with a twist: When you have picked a non-matching pair, the two tiles you have chosen switch.</p>

    <!-- OPTIONAL: include this if you want history support -->
    <iframe src="javascript:''" id="__gwt_historyFrame" tabIndex='-1' style="position:absolute;width:0;height:0;border:0"></iframe>

    <noscript>
      <div style="width: 22em; position: absolute; left: 50%; margin-left: -11em; color: red; background-color: white; border: 1px solid red; padding: 4px; font-family: sans-serif">
        Your web browser must have JavaScript enabled
        in order for this application to display correctly.
      </div>
    </noscript>

    <div id="gameContainer"></div>
  </body>
</html>

Questions

The main concern I have is: Is it somehow possible to cheat by debugging the Javascript variables to figure out the values of the tiles in advance? I am considering to make some more GWT stuff, but if it is possible to cheat then I will have to alter the way that I am making it (generate stuff dynamically for example, right before you actually make a move).

Other than that: How is my use of GWT?

Other than that: Any comments welcome. I am not very focused on the HTML and CSS stuff though, they are just included for the sake of completeness.

share|improve this question
1  
Amazing---I created this exact game, with the exact same name, several years ago while learning to program. vivatropolis.org/istvan/MemoryTwist.xhtml –  Istvan Chung 11 hours ago

2 Answers 2

Hmm, very nice! Nice exercise for memory (despite the bit, uhm, sadistic twist), and the code is nicely written and very hard to pick on: what can be final is final, arguments are validated, no magic numbers or duplicated string literals, well-formatted and clear, easy to understand. I noticed you pick off the last element from the shuffled list, and even the HTML passes the w3c validator.

As a minor optimization, I would recommend removing the click listeners from the matched tiles that were revealed.

The Random object is used only once, so it could as well be a local variable, or better yet, just embedded in the statement where you use it. Later if you want to make this testable, you can consider adding it as a constructor argument. In any case, in the current code there's no visible reason to make it a member field.

Although it's clever to pop off the elements of ints from the end, how about not removing anything at all:

for (int x = 0; x < width; x++) {
    for (int y = 0; y < height; y++) {
        int value = ints.get(x * width + height);

Although this is more "efficient" in theory, in your use case this is practically negligible. You could as well use the short and sweet and inefficient ints.remove(0), it still wouldn't make a practical difference.

A minor usability note: the numbers on the memory cards are 0-based, on the example page ranging from 0 to 17. That's fine for geeks, but regular human beings would probably expect 1-18 instead.

share|improve this answer
4  
Your suggestion about removing click listeners from matched tiles is not only a "minor optimization", it solves a bug ;) –  Simon André Forsberg 15 hours ago
2  
I thing that regular human beings wouldn't expect numbers in a memory game at all, but pretty pictures. So I think that starting with 0 is fine for numbers. –  tim 14 hours ago

Switch negative if statements

I would switch the if statements around in code like this:

if (!condition) {
    // long block of code
} else {
    // short block of code
}

You have this twice: previousClicked != null and !same.

I would do this for two reasons: it's easier to read if(condition) than if(!condition), and because the shorter code is at the top it also becomes easier to see what else statements finish what if statements when you have nested statements.

Usability

I would add a functionality that hides the two open tiles immediately if the user clicks anywhere, instead of ignoring the click and letting the timer finish. It might just be that I am impatient, but it is quite annoying for me to wait out the timer.

setWidget

grid.setWidget(y, x, view);

Should this be grid.setWidget(x, y, view);?

Is it somehow possible to cheat by debugging the Javascript variables to figure out the values of the tiles in advance?

This would be a lot easier to test with non-obfuscated code. If you change your example, I'm sure someone would try to cheat it.

share|improve this answer
1  
About grid.setWidget: No, it should be like that. (I know, it feels weird...) –  Simon André Forsberg 14 hours ago
    
As for the obfuscated code: I can provide a non-obfuscated version as well (at a later time), but what I will actually use for any game such as this, is of course obfuscated code. –  Simon André Forsberg 14 hours ago

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.