Code review

[adriansoghoian]

Hey @openspectrum, here's our project's code for review (FYI master is fully updated and is the one to review).

A few areas to draw your attention to:

• Our JS file is enormous :) Would you recommend breaking up? If so, how? Perhaps mapping to our controllers or models (e.g. game, chat)?
• I tried to add OO structure to chat by creating a Conversation constructor. Not that much logic is contained there though; in general how would you recommend we refactor our client logic to be more modular?
• Any other feedback would be greatly welcomed!

Thanks!

PS. The JS for the Google Data Viz isn't rendering, but it's a known (and solveable) issue we'll tackle soon ;)

PPS. A general apology for the state of the code. Please be gentle.

[tannerwelsh]

Answering question 1.

Yes, there are many changes to be made to your JS. First off, there seems to be no explicit dependency between the game logic and the chat logic (nor should there be), so those two should be decoupled as much as possible.

To clean this up, I'd suggest defining a much more explicit object model. Here is one way to break it down:

// in player.js
Player
Player.get() // returns a new Player object with data from server (via AJAX)

// in game.js
Game
Game.board // returns a board object representing the state of the board
Game.isOver // state control
Game.lastMove
Game.move()
Game.listenForMove

Board
Board.get()
Board.update()
Board.vertical(), Board.diagonalUp(), etc.


// in chat.js
Chat
Chat.messages
Chat.register() // register a player as a chat participant
Chat.sendMessage()
Chat.refresh()

It looks like Player is an object that both Game and Chat would depend on to function. So ideally it would be extracted to it's own module and then loaded dynamically using a system like require.js. This is a better way to do dependency management than the other options in client-side JavaScript, which are:

1. Put everything in one giant file (or a giant complied file)
2. Use linear script loading in the browser to organize dependency, like this:

<script src="/js/application.js"></script>
<script src="/js/player.js"></script>
<script src="/js/game.js"></script>
<script src="/js/chat.js"></script>
<script src="/js/googlevisualization.js"></script>

Where the player.js script does the work of building a new Player object, which can both store state of a currently "registered" player and also fetch player data from the server. If it is loaded in this way, then game.js and chat.js will have access to any globally defined variables from player.js, like Player.

[tannerwelsh]

All these variables related to the game, just hanging out on their own without being encapsulated in objects. Lost in the big world. So sad. [sigh]

[tannerwelsh]

Answering question 2.

Adding OO design principles to chat on the server and the client is fairly straight-forward. I think you are on the right track with the following schema:

Messages belong to players.
Conversations have many messages.
Players have many conversations.
Conversations have many players (users).

For a clean symmetry, you could implement this data model on both the server and the client.

[tannerwelsh]

General feedback: I think you all know how to write cleaner code than this. I like that you're playing with multiple technologies here (data viz, chat, game mechanics), but as such there should be much cleaner boundaries between them.

[tannerwelsh]

Also, for extra fun times, you might want to check out websockets. Here's a blog post that covers how to use them from Ruby: https://blog.engineyard.com/2013/getting-started-with-ruby-and-websockets