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"); } });

You have id="lobby-user" on an ng-repeat. This is invalid; you cannot have more than one HTML element with the same ID. Use a class! — Dan
– Dan, Commented Aug 10, 2015 at 9:51

Dan · Accepted Answer · 2015-08-10 09:55:21Z

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.

I have to stupidly disagree with your recommendations involving the CardshifterServerAPI. I say "stupidly" because my knowledge is AngularJS is poor. This line new CardshifterServerApi.messageTypes.ServerQueryMessage("USERS", "") is creating a new ServerQueryMessage as strictly defined in the Cardshifter server code itself. This properly formats a message so the code does not have to hardcode and repeat any JSON code. and, creating an entire service to just send a single type of message out of many other types? That seems unnecessary. Please respond to this message in the chat room. — SirPython
– SirPython, Commented Aug 11, 2015 at 1:37
No, you're right, you don't want to create one service per message. However, you do want to create a service for each logical grouping of messages. For example you might have a service for users, for cards, and so forth. You probably want this service to be aware of how to create the messages but not the implementation of them. This is perfect for the factory pattern. — Dan
– Dan, Commented Aug 11, 2015 at 7:48
Is it still okay to use $scope considering this is not version 2.0, and probably won't be for a while, considering 2.0 is going to have (at least) a few features built around ES6? And, would it be okay to use this instead, or would using controllerAs be better? — SirPython
– SirPython, Commented Aug 19, 2015 at 20:42
There's nothing wrong with using $scope, but you will end up having to make sure you always put stuff on an object in scope anyway , so might as well use controlleras. If you don't use an object on your scope then you run into issues with prototypal inheritance and isolated scopes. controllerAs fixes this, IMO — Dan
– Dan, Commented Aug 19, 2015 at 20:45

Quill · Accepted Answer · 2015-08-09 05:58:41Z

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:

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.
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.
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.
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:

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.

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!

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.

You repeated var ENTER_KEY = 13; twice.

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!

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;.

If he's repeating var ENTER_KEY = 13; and not noticing, he's not using strict mode. That would be my first suggestion, regarding JS. — ANeves
– ANeves, Commented Aug 9, 2015 at 13:49
Regarding the CSS "wacky indentations": it's not an uncommon indentation style for CSS. In fact, it's one of the 4 output settings offered by Sass (nested). — cimmanon
– cimmanon, Commented Aug 9, 2015 at 16:52
I personally don't see the harm in the empty CSS classes. They're convenient place holders reminding you which classes you defined, but didn't specify a style for. — RubberDuck
– RubberDuck, Commented Aug 9, 2015 at 17:50
@RubberDuck They don't cause immediate harm, but they cause a hit in performance. Also, you can easily replace them with a comment like /* TODO: add a background on #lobby-deck-builder */ and you would be set. — Ismael Miguel
– Ismael Miguel, Commented Aug 9, 2015 at 18:44
I think the performance hit of having an empty class is negligible, personally... — Dan
– Dan, Commented Aug 10, 2015 at 9:49

cimmanon · Accepted Answer · 2015-08-09 17:15:12Z

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; }

"This is a clear and obvious misuse of the table element." A harsh but honest remark, I was honestly hoping you'd see this question and point out things like that! Do you think using divs instead of table elements would make sense? — Phrancis
– Phrancis, Commented Aug 9, 2015 at 17:28
Well, div doesn't carry any semantic meaning with it. You could use it and it would be fine, but you would almost certainly be better off using the new HTML5 sectioning elements (section, aside, header, footer, etc.). — cimmanon
– cimmanon, Commented Aug 9, 2015 at 17:31
Hm, never heard of those. Any compatibility issues at all between browsers? Do you know a link to good documentation for them? — Phrancis
– Phrancis, Commented Aug 9, 2015 at 17:33
Actually, older browsers will treat them like spans, not divs (divs actually have styles applied to them by default: display: block). The only browsers that are a problem are IE8 and older (see: en.wikipedia.org/wiki/HTML5_Shiv). Every other browser will allow you to style unknown elements the same way you would style any others. For HTML5 element information, I like HTML5 Doctor. — cimmanon
– cimmanon, Commented Aug 9, 2015 at 18:10
Angular (post 1.3 - you are using Angular 1.3+ right?) does not support IE8 or earlier so this point is moot. +1 for HTML5 doctor, I have a flowchart by them printed on my desk. — Dan
– Dan, Commented Aug 10, 2015 at 9:37

Quill · Accepted Answer · 2015-08-09 05:48:04Z

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: possible bug

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.

Nice catch on the "Phrancis" name twice, looks like I had connected two instances under the same user name, which is something we're going to implement on preventing shortly. :) — Phrancis
– Phrancis, Commented Aug 9, 2015 at 7:27
Thanks for the recommendations you added at the end! About the console.logs: Those are temporary. Other parts of the application are still being worked on, and these are the least important parts. Therefore, these are being used as reminders (and as "breakpoints" in testing). — SirPython
– SirPython, Commented Aug 9, 2015 at 14:16
I don't see any benefit to reordering the CSS properties by any method you're recommending. The extra whitespace between the closing tr/table tags can easily be attributed to quirks in the templating engine and is rarely worth concerning yourself over. — cimmanon
– cimmanon, Commented Aug 9, 2015 at 16:56

3 revs, 2 users 91% · Accepted Answer · 2015-08-17 02:11:29Z

9

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) { }).

edited Aug 17, 2015 at 2:11

community wiki

3 revs, 2 users 91%
ANeves

\$\begingroup\$ I think your answer needs more meat to it. Explaining a little better what it does, how to do it and how it will prevent errors will be useful. \$\endgroup\$

Ismael Miguel
– Ismael Miguel

2015-08-10 09:46:20 +00:00
Commented Aug 10, 2015 at 9:46
\$\begingroup\$ @IsmaelMiguel I agree that it needs more meat. I improved it, but also made it CW to entice others to contribute. \$\endgroup\$

ANeves
– ANeves

2015-08-10 10:19:22 +00:00
Commented Aug 10, 2015 at 10:19
\$\begingroup\$ That's a lot better. You could have waited 3 minutes before making it into a community wiki, so I could give you +10 \$\endgroup\$

Ismael Miguel
– Ismael Miguel

2015-08-10 10:21:07 +00:00
Commented Aug 10, 2015 at 10:21
1

\$\begingroup\$ It's fine, I won't get poor from that. :p \$\endgroup\$

ANeves
– ANeves

2015-08-10 10:24:01 +00:00
Commented Aug 10, 2015 at 10:24

Add a comment |

Stack Exchange Network

Cardshifter game lobby

5 Answers 5

`id` attributes

Inline style.

`$scope`

HTML validation

`window` access

Separation of concerns

CardshifterAPI

Tables for layout

`!important`

`strict mode`

`$parent`

On controllers in general

`value='{{mod}}'`

HTML:

CSS:

JavaScript:

Tables for layout

Excessive IDs

`!important` when you don't need to

Invalid CSS

Learn to love shorthand

The Mundane:

Everything else:

You must log in to answer this question.

Linked

Hot Network Questions

Cardshifter game lobby

5 Answers 5

id attributes

Inline style.

$scope

HTML validation

window access

Separation of concerns

CardshifterAPI

Tables for layout

!important

strict mode

$parent

On controllers in general

value='{{mod}}'

HTML:

CSS:

JavaScript:

Tables for layout

Excessive IDs

!important when you don't need to

Invalid CSS

Learn to love shorthand

The Mundane:

Everything else:

You must log in to answer this question.

Linked

Related

Hot Network Questions

`id` attributes

`$scope`

`window` access

`!important`

`strict mode`

`$parent`

`value='{{mod}}'`

`!important` when you don't need to