citizengit/loople — pull request #13 by mathildathompson (2014-04-24 03:16:06+00:00)
Title: Project_2_feeback

Hi Loople Team,

Your project was great! Very cool, am very impressed with what you managed to achieve in a week.

Kriss you may not want to merge this pull request into the project, but it could be helpful for you to look at some of the refactoring I have done (you were probably going to do most of this anyway as I know you were in a rush the last couple of days).

The main thing is the structure of the Javascript files. It makes it very hard to follow having individual functions in their own files, I have grouped the functions together into two objects. The first is the startPage object (contains all functions that need to be run on page load and all timers) the second is a musicLoop function which has all loop related functions. Look at these functions and try and have a go at refactoring further.

The second point which Kriss and I discussed yesterday is the structure of the loop data throughout the app. You need to keep it consistent. I would advise keeping it like this:

var Loop = {
id: 30,
name: "First Loop",
keyStrokes: {
3: ["1", "3"],
55:["10, "17"]
}
}
You simplifies this into a hash with id, name and time as keys, this makes the validation difficult and I think would cause problems when you try build on the application.

You need to build in validations. At the moment, you are only validating the user on the server side. Build both clientside and server side validations for loops and keystrokes.

The app is not quite fully crud, have a go at implementing the update function for loops.

You definitely need to use underscore or handlebars templating here, there is too much html in your JS files. And put the signin and signup form into a partial to clean up that view.

Now the app is cleaner I think it would be nice to try and add Charlies shaped back in cause they were cool!

Excellent work guys. People are going to be impressed. Let me know if you have any questions with the refactor!