Cardshifter game lobby

Question

We've been hard at work creating a browser-based GUI/Client for playing the Cardshifter TCG for the past week or so. Today, I just finished doing a pretty complicated layout for the chat lobby, along with @SirPython who put together the JavaScript/AngularJS code, which I will include with his permission.

Here's the full repo on GitHub, in case you're interested.

I have not worked with table-based HTML/CSS layouts for a long time, and I wanted to know if the code was well-structured, if I'm using CSS and Bootstrap efficiently, etc.

Here is lobby.html. Note that the sections of text {{in brackets}} are injected by Angular. The header and other things are in a separate "mother" HTML file, where this child page (and others) get injected with the flow of the game.

 <table id="lobby">
    <tr id="lobby-headers">
        <td id="lobby-title">Cardshifter Lobby</td>
        <td id="lobby-deck-builder" width="20%"><button class="btn btn-navbar csh-button">Deck Builder</button></td>
    </tr>
    <tr id="lobby-invite-request" ng-show="gotInvite">
        <td colspan="2">
            <div id="lobby-accept-invite">
                Game invite from {{invite.name}} to play {{invite.type}}!<br/>
                <input ng-click="acceptInvite(true)" type="button" value="Accept" class="btn btn-success"/>
                <input ng-click="acceptInvite(false)" type="button" value="Decline" class="btn btn-danger"/>
            </div>
        </td>
    </tr>
    <tr id="lobby-list-headers">
        <th id="lobby-message-list-header">Messages</th>
        <th id="lobby-users-list-header">Users Online</th>
    </tr>
    <tr id="lobby-lists" height="400px">
        <td id="lobby-message-list">
            <ul id="lobby-chat-messages">
                <li ng-repeat="message in chatMessages" id="lobby-chat-message">
                    [{{message.timestamp}}] {{message.from}}: {{message.message}}
                </li>
            </ul>
        </td>
        <td id="lobby-users-list">
            <ul id="lobby-users">
                <li ng-repeat="user in users" id="lobby-user">
                    <input ng-model="$parent.$parent.selected_opponent" ng-if="user.userId!=currentUser.id" type="radio"
                           value="{{$parent.user.userId}}" name="user_selection" /> {{user.name}}
                </li>
            </ul>
        </td>
    </tr>
    <tr>
        <td id="lobby-message">
            <textarea ng-model="user_chat_message" ng-keyup="sendMessage($event)"
                id="lobby-chat-text-area" rows="1" cols="75" wrap="off"
                placeholder="Enter chat message..."></textarea>
            <input ng-click="sendMessage()" ng-disabled="sending" type="submit" value="Send" class="btn btn-navbar csh-button"/>
        </td>
        <td id="lobby-invite">
            <input ng-click="startGame()" type="button" value="Invite to game" class="btn btn-warning"/>
        </td>
    </tr>
    <tr id="lobby-mods">
        <td colspan="2" id="lobby-mod-selection">
            <form class="form-inline" role="form">
                <div class="form-group">
                    <label for="mod_selection">Select game type:</label>
                    <div ng-repeat="mod in mods" class="form-control" id="lobby-mod-selector">
                        <input ng-model="$parent.selected_mod" type="radio" value="{{mod}}"
                            name="mod_selection" id="mod_selection"/>
                        {{mod}}
                    </div>
                </div>
            </form>
        </td>
    </tr>


</table>

Here is lobby.css which is linked to it. Note there are a number of empty selector classes, which all exist but some are not being used at the moment, and may be in the future if needed.

/* WHOLE LOBBY */

#lobby {
    width: 100%;
}

/* TABLE HEADERS */

#lobby-headers {
    font-family: Georgia, Times, "Times New Roman", serif;
    text-align: center;
    color: #DDDDDD;
    background-color: #000000;

}

#lobby-title {
    font-size: 1.5em;
    font-weight: bold;
}

#lobby-deck-builder {}

/* SECTION HEADERS */

#lobby-list-headers {
    font-family: Georgia, Times, "Times New Roman", serif;
    font-size: 1.2em;
}

#lobby-message-list-header {
    text-align: center;
}

#lobby-users-list-header {
    text-align: center;
}

/* MAIN MESSAGE & USERS SECTIONS */

#lobby-lists {
    vertical-align: text-top;
}

#lobby-message-list {
    font-size: 0.8em; !important
}
    /* List of all messages */
    #lobby-chat-messages {
        list-style-type: none;
        padding-left: 0;
    }
        /* Each individual message line */
        #lobby-chat-message {
        }

#lobby-users-list {
    font-size: 0.9em;
    font-family: Georgia, Times, "Times New Roman", serif;
}
    /* List of all users */
    #lobby-users {
        list-style-type: none;
        padding-left: 0;
    }
        /* Each individual user line */
        #lobby-user {
        }

/* FOOTER SECTIONS */

#lobby-message {
    background-color: #000000;
    vertical-align: bottom;
}
    /* TEXT AREA FOR TYPING CHAT MESSAGES*/
    #lobby-chat-text-area {
        outline: none;
        overflow: auto;
        vertical-align: middle;
    }

#lobby-invite {
    background-color: #000000;
    text-align: center;
}

#lobby-mods {}

#lobby-mod-selection {}

    /* DIV CONTAINING RADIO BUTTON AND MOD NAME */
    #lobby-mod-selector {
        border: 1;
    }

/* Game invite accept dialog */
#lobby-invite-request {
    font-family: Georgia, Times, "Times New Roman", serif;
    font-size: 1.6em;
    text-align: center;
    background-color: #0033CC;
    color: #EEEEEE;
    border-top-color: #FFFFFF;
    vertical-align: middle;
}

Here is the lobby_controller.js, it is AngularJS written by @SirPython, which links the client to the server messages:

CardshifterApp.controller("LobbyController", function($scope, $timeout) {
    var CHAT_FEED_LIMIT = 10;
    var ENTER_KEY = 13;
    var MESSAGE_DELAY = 3000;
    var ENTER_KEY = 13;

    $scope.users = [];
    $scope.chatMessages = [];
    $scope.mods = [];
    $scope.currentUser = window.currentUser;
    $scope.invite = {
        id: null,
        name: null,
        type: null
    };
    $scope.gotInvite = false;


    var commandMap = {
        "userstatus": updateUserList,
        "chat": addChatMessage,
        "inviteRequest": displayInvite,
        "availableMods": displayMods,
        "newgame": enterNewGame
    };

    var getUsers = new CardshifterServerAPI.messageTypes.ServerQueryMessage("USERS", "");
    CardshifterServerAPI.sendMessage(getUsers);

    CardshifterServerAPI.setMessageListener(function(message) {
        commandMap[message.command](message);
        $scope.$apply(); // needs to manually updated since this is an event
    }, ["userstatus", "chat", "inviteRequest", "availableMods", "newgame"]);

    $scope.sendMessage = function(e) {
        if(e && e.keyCode !== ENTER_KEY) { // user may hit "enter" key
            return;
        }

        $scope.sending = true;
        var chatMessage = new CardshifterServerAPI.messageTypes.ChatMessage($scope.user_chat_message);
        CardshifterServerAPI.sendMessage(chatMessage);

        $scope.user_chat_message = ""; // clear the input box
        $timeout(function() { // allow another message to be sent in 3 seconds
            $scope.sending = false;
        }, MESSAGE_DELAY);
    }

    $scope.startGame = function() {
        if($scope.selected_mod && $scope.selected_opponent) {
            var startGame = new CardshifterServerAPI.messageTypes.StartGameRequest($scope.selected_opponent,
                                                                                   $scope.selected_mod);
            CardshifterServerAPI.sendMessage(startGame);
        } else {
            // user needs to choose an opponent and/or a mod
            console.log("need to choose mod and/or opponent");
        }
    }

    $scope.acceptInvite = function(accept) {
        var accept = new CardshifterServerAPI.messageTypes.InviteResponse($scope.invite.id, accept);
        CardshifterServerAPI.sendMessage(accept);
        $scope.gotInvite = false;
    }


    // The command map functions:
    /**
    * Based on the content of message, will add or remove
    * a user from the user list.
    */
    function updateUserList(message) {
        if(message.status === "OFFLINE") {
            for(var i = 0, length = $scope.users.length; i < length; i++) {
                if($scope.users[i].userId === message.userId) {
                    $scope.users.splice(i, 1); // remove that user from the array
                    break;
                }
            }
        } else {
            $scope.users.push(message);
        }
    }
    /**
    * Adds a chat message to the message feed. If the message
    * feed is at the maximum limit of messages, deletes the oldest
    * message.
    */
    function addChatMessage(message) {
        if($scope.chatMessages.length === CHAT_FEED_LIMIT) {
            // remove the oldest chat message
            $scope.chatMessages.shift();
        }

        var now = new Date();
        var YMD = now.getFullYear() + "-" + (now.getMonth() + 1) + "-" + now.getDate();
        var HMS = now.getHours() + ":" + now.getMinutes() + ":" + now.getSeconds();
        message.timestamp = YMD + " " + HMS;

        $scope.chatMessages.push(message);
    }
    /**
    * Shows buttons and a message to this client for accepting
    * or declining a game request.
    */
    function displayInvite(message) {
        $scope.invite.id = message.id;
        $scope.invite.name = message.name;
        $scope.invite.type = message.gameType;
        $scope.gotInvite = true;
    }
    /**
    * Shows to the user a list of all available mods.
    */
    function displayMods(message) {
        $scope.mods = message.mods;
    }
    /**
    * Stores the game ID in currentUser for other controllers
    * to use and navigates to the deck-builder page for the
    * user to select a deck.
    */
    function enterNewGame(message) {
        currentUser.currentGameId = message.gameId;
        console.log("change to game");
    }
});

Answer 1

Background: I work as a web developer at a company. I'm the only Angular guy here, and we are using Angular in our greenfield CRM project, so my answer will mainly address Angular (as no one else has here).

id attributes

First thing is first - you're using lots of id attributes. Generally speaking, you only want to use id when it

• Is a unique 'object' in HTML. After all, IDs cannot be repeated.
• The use-case mandates it.

You should not use IDs for style hooks. I personally avoid using them entirely, unless I want to use same-page anchors - I prefer to use classes prefixed with js-* or, when I have access to Angular (like you do), I prefer to use directives, when I want JavaScript interactivity.

Inline style.

Inline styling is bad practice - you really want to separate presentation and content where possible, and that includes separating your CSS from HTML as much as possible. Use CSS rules to apply any styling. You can use any methodology you like - I prefer the BEM model of CSS, but the Bootstrap model is fine as well or semantic HTML also works.

This also goes for style properties - for example, height and width are both style properties. These should be in the CSS file.

$scope

Don't use $scope where possible. It's being removed in Angular 2.0 and makes your code hard(er) to reason about. Instead, use the controllerAs syntax. This uses $scope under the hood, but ensures that you don't fall into any of the nasty prototypical pot-holes Angular throws your way when you use isolation scopes. It also makes your templates easier to reason about because you can clearly associate things - lobby.user makes a lot more sense than just user.

HTML validation

I do not swear by using data-ng-* attributes. When you use directives you are essentially creating custom elements, which, by specification, have attributes that are not prefixed by data-*. In addition, browsers ignore unknown attributes - take care to not make an attribute with the same name as one in the specification and you're fine, in my opinion. If you are going to prefix everything with data-ng-*, then do it in a build step; angular will strip out any data- prefixes automatically.

window access

Never access window, especially not from within a controller. window is DOM access, which is bad; use $window. DOM access should be restricted solely to directives. You are allowed to use $window in the controller, although you almost certainly want to use it in a directive instead.

$window is Angular's own version of window which is wrapped in its dependency injection container - this allows you to mock it should you wish. You can't do any such thing with window.

Separation of concerns

Your Angular code is not very separate at all. Your controller is doing a lot of things. For example you have a lot of constants - CHAT_FEED_LIMIT - that should really be angular constants instead. As I mention below, controllers should only glue the UI and any user state with the rest of the application; they should be pretty dumb and do no work, similar to the MVC model on the server side.

You want to split your code as much as possible so that you only have one responsibility per thing.

CardshifterAPI

This seems like a namespace on window. You shouldn't rely on things being in window with Angular. You have a killer dependency injection container, and you should definitely use it. In addition, new CardshifterServerApi.messageTypes.ServerQueryMessage("USERS", "") is strange. I don't know what this does (I've looked briefly into your source but couldn't ascertain), but it looks very weird (to me), and this ties into the dependency injection note I made; you could very easily use Angular to inject this as ServerQueryMessage and then do new ServerQueryMessage() instead. This looks a lot nicer and eliminates the requirement for your window dependency in this instance.

Your CardshifterAPI itself seems like it has too many responsibilities. I realize it is a self-contained script that is loaded before the rest of Angular code. Relying on code order is a maintenance nightmare - I would recommend using webpack or browserify and/or trying to convert your CardshifterAPI file into an Angular module.

Tables for layout

No. Use tables for tabular data.

!important

No. Don't use !important. !important is almost always a sign of not understanding the cascading behaviour of CSS. The only time you ever want to use !important is when you need to override something in 3rd party code that you cannot override usign specifity.

strict mode

Turn it on, learn it, love it. It gives you lots of benefits for free and forces you to write standards-compliant javascript code.

$parent

Access of $parent is again, usually an indication you're doing it wrong(tm). Use directives in tandem with isolation scopes. If you use $parent you directly couple the calling site to the heirarchy of your scopes which makes your code non-reusable. Instead of

<div ng-repeat="mod in mods" class="form-control" id="lobby-mod-selector">
    <input ng-model="$parent.selected_mod" type="radio" value="{{mod}}"
        name="mod_selection" id="mod_selection"/>
    {{mod}}
</div>

Create a directive that takes selected-mod as an argument, then assign it to the scope using two-way binding. Here's an example:

module.directive('modList', function() {
  return {
    scope: {
      model: '='
    },
    controller: function($scope, mods) {
      $scope.mods = mods;
    },
    template: '<div ng-repeat="mod in mods"><mod moderator="mod" model="model"></mod></div>'
  };
})
.directive('mod', function() {
  return {
    scope: { 
      moderator: '=',
      model: '='
    },
    template: '<input type="radio" ng-value="moderator" name="mod_selection" ng-model="model"> {{ moderator }}'
  }});

This is then called a bit like this:

<mod-list model='selected_mod'></mod-list>

Obviously, this could be improved, but this looks a lot more semantic. Caveat: You cannot dynamically generate names, so you won't be able to use expressions with the name attribute. In essence, this means that you would not be able to reuse this directive for anything else (no dynamic radio lists).

On controllers in general

I'm of the opinion that there's no reason to have standalone controllers any more. As you may have noticed in my above example, I sepcify the controller directly in line. This is because in Angular 2.0 controllers are going away (as is $scope). Controllers are almost always only ever coupled with a route, directive or ng-controller and there's never really a need to test them in an isolated environment because they should only be used to glue UI code to service code. As a result, I also feel that your current controller is essentially a god class and you should refactor a lot of the code out of that into services.

For example, acceptInvite knows how to both construct the message it needs and send it. This should instead call a service that does all of the heavy lifting; the only thing that should be in controller is UI state (again, in my opinion). For example, instead of

$scope.acceptInvite = function(accept) {
  var accept = new CardshifterServerAPI.messageTypes.InviteResponse($scope.invite.id, accept);
  CardshifterServerAPI.sendMessage(accept);
  $scope.gotInvite = false;
}

I would prefer this

$scope.acceptInvite = function(acceptOrDecline) {
  // inviteService is a bit contrived, you'd probably want this in some other service
  // but it is an example
  $scope.inviteService.respondToInvite($scope.invite.id, acceptOrDecline).then(function() {
    $scope.gotInvite = false;
  });
}

value='{{mod}}'

This won't work like you expect; use ng-value instead. Before angular loads, value will have the value of {{mod}}. ng-value ensures that this value isn't set if Angular doesn't load. Also, that lets you use an expression isntead of having to specify {{}}. For example, value="{{mod}}" becomes ng-value="mod".

---

This is all I can think of right now. The main take aways are to use directives, remove inline styling. There's more I'd do, but I have limited time :-) Maybe I will contribute.

Answer 2

Well, there's lots of code needing review. I've done my best to split it into each language. I've avoided reviewing Angular, since it's a library I'm not comfortable with.

HTML:

1. You have tons and tons of ng-* properties.

   That is invalid HTML. You must use data-ng-*.

   According to the documentation, data-ng-* is valid and will be detected.

2. Still using <td width="foo"> and <td height="foo">? Are we in the 90s?

   Those attributes are deprecated since HTML 4.01, use CSS instead.

   Also, 400px is an invalid value. You have to specify the value without px (Yes, it only supports pixels and percentages).

   Please, use CSS.

3. You have sparse fields everywhere!

   There is not even a single <form>!

   Oh, wait! There is this: <form class="form-inline" role="form">, but it doesn't even have a submit button or anything.

4. Some ids need some re-wording.

   One example is id="lobby-chat-text-area". I know it is a textarea! What is it for? To make toasts? No! It's a message! For this example, changing to id="lobby-chat-message" would be better.

---

CSS:

1. You have empty styles!

   For example: #lobby-deck-builder {}.
   If they are empty, it means they aren't doing anything. If they aren't doing anything, you can remove them.

2. You have wacky indentations.

   Look at this mess:

   #lobby-message-list {
       font-size: 0.8em; !important
   }
       /* List of all messages */
       #lobby-chat-messages {
           list-style-type: none;
           padding-left: 0;
       }
           /* Each individual message line */
           #lobby-chat-message {
           }
   

   While it makes it easier for you, it complicates stuff for others. Please, don't do this!

3. Repeated styles.

   I'm sure you know what DRY means.

   You have the following lines:

   #lobby-message-list-header {
       text-align: center;
   }
   
   #lobby-users-list-header {
       text-align: center;
   }
   

   When, you could just write this:

   #lobby-users-list-header, #lobby-message-list-header {
       text-align: center;
   }
   

   Or move them into a class. The same applies for font-family.

---

JavaScript:

There isn't much to be said about your JavaScript.

1. You repeated var ENTER_KEY = 13; twice.

2. You are misusing objects.

   You have the following:

   var commandMap = {
       "userstatus": updateUserList,
       "chat": addChatMessage,
       "inviteRequest": displayInvite,
       "availableMods": displayMods,
       "newgame": enterNewGame
   };
   

   Why don't you just attribute the functions here? Objects are good for that too!

3. You lost something on your function, updateUserList.

   You have left a break; behind. The break there is quitting the for loop. You can simply use return;.

Answer 3

Tables for layout

This is a clear and obvious misuse of the table element. Tables are for representing tabular data and that's it. Would that same content make sense in a spreadsheet? If the answer is no (as it is in this case), then don't use a table. Use CSS for controlling your layout instead.

Excessive IDs

You don't have a single non-id selector in your CSS. That is some serious code smell. By doing this, you're essentially implying that each element in the document has absolutely nothing in common with anything else. Really? There's no place where an element selector would be appropriate (psst, if you were using semantically appropriate markup rather than tables and divs, you would actually have different elements you could use as style hooks)? Or a class? Or a descendant selector?

!important when you don't need to

You're only using ids throughout your stylesheet and everything has the same specificity. There are very very few cases where !important is appropriate and this isn't one of them.

#lobby-message-list {
    font-size: 0.8em; !important
}

Invalid CSS

You must have a unit for border (border-width) unless the value is 0.

#lobby-mod-selector {
    border: 1;
}

Learn to love shorthand

#lobby-invite-request {
    font-family: Georgia, Times, "Times New Roman", serif;
    font-size: 1.6em;
}

Becomes

#lobby-invite-request {
    font: 1.6em Georgia, Times, "Times New Roman", serif;
}

Unless you need to preserve previously declared values, you're going to get more compact CSS by using shorthand.

#lobby-chat-messages {
    list-style-type: none;
}

Becomes

#lobby-chat-messages {
    list-style: none;
}

Answer 4

There's not much to cover, other than the things @IsmaelMiguel went over, nonetheless:

---

The Mundane:

---

This is less of a important thing, as a style thing, but:

font-family: Georgia, Times, "Times New Roman", serif;
font-size: 1.6em;
text-align: center;
background-color: #0033CC;
color: #EEEEEE;
border-top-color: #FFFFFF;
vertical-align: middle;

I find it better to stack things alphabetically, or alphabetically in groups (for example: align, colors, fonts, text), or, you can stack them by line length. This point also applies to JS as well.

---

A little more relating to general style:

// user needs to choose an opponent and/or a mod
console.log("need to choose mod and/or opponent");

It's better to keep the order of things the same for lists, rather than swapping them. Also, the .log text is pretty clear, the comment is somewhat superfluous.

---

    </tr>


</table>

You have an extraneous line here, that's alright if that's your style, however, keep it a constant pattern throughout your code.

---

Not exactly sure what's up with this:

But, if it's a bug, you may want to look at that.

---

console.log("change to game");

Slightly pointless, and deserves a capitalisation and period.

---

Everything else:

---

var YMD = now.getFullYear() + "-" + (now.getMonth() + 1) + "-" + now.getDate();
var HMS = now.getHours() + ":" + now.getMinutes() + ":" + now.getSeconds();

Instead of concatenating like that, I find that it's better to join instead.

var YMD = [now.getFullYear(), now.getMonth() + 1, now.getDate()].join("-");
var HMS = [now.getHours(), now.getMinutes(), now.getSeconds()].join(":");

You can even do it with the final combination:

message.timestamp = [YMD, HMS].join(" ");

It's also a little easier if you want to expand or whatnot, later on.

---

CardshifterServerAPI.setMessageListener(function(message) {
// ...
}, ["userstatus", "chat", "inviteRequest", "availableMods", "newgame"]);

You should avoid having to manually update that, instead, by fetching the keys from the object. Something like this would work:

var keys = [];
for(var k in commandMap){ keys.push(k); }
CardshifterServerAPI.setMessageListener(function(message) {
// ...
}, keys);

---

Like Chat.SE, the @ pinging system could be implemented, similarly with a sound, and highlighted name. And similarly with Chat.SE, giving the server chat a special colour like mods have on Chat.SE would be nice, otherwise I trouble-makers might be tempted to sign up with 'Server Chat 2' as a username and confuse everyone.

Answer 5

You should use strict mode in your JS scripts. It will allow you to avoid a lot of common mistakes.

See the section "Changes in strict mode" in the MDN article I linked.
Some of the advantages of using strict mode:

• Disallows global variables (when dev "forgets" to declare them);
• Disallows duplicate variables (such as declaring ENTER_KEY twice);
• Function parameters must be unique (function foo (x, x) { }).