Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

files for code review #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

files for code review #2

wants to merge 2 commits into from

Conversation

jplehmann
Copy link
Owner

I have removed the files I would like to be reviewed so they will show up in the pull request, and you can comment line-by-line.

@adamstrickland
Copy link

Looks functional. As your application gets more complex you're going to have to deal with javascript complexity, which is something to be aware of and to prepare for; the existing frameworks out there go a long way to making that easier, but it can still be a challenge. Related to that would be to read up on AMD and related loaders, like RequireJS or consider using a consolidation tool; in Ruby there's one called sprockets, that reads a manifest file and consolidates all your JS into one file. Sass/Compass and LESS are similar tools for managing CSS complexity (I prefer compass).

I do have one critique regarding the JS: it looks like a Perl script. Consider structuring your code into classes/objects. I know it sounds trivial, but it's also a bit weird in JavaScript. A simple way to do so looks like:

function FooClass(options){
  var self = this;
  var config = options || {};

  self.doFoo = function () {
    ...
  };

  self.doFooToo = function (n) {
    ...
    var v = config.v || 123;
    ...
  };

  return self;
}

var foo = new FooClass();
foo.doFoo();
...

Be aware, especially in JS, there's more than one way to do all this...

Anyway, that should remove most (all?) of the function declaration problems you might have been having.

Also, you mentioned Underscore; it's got a lot of nice stuff that really makes JS much nicer. Simply being able to do this: _.map(collection, function (item) { return item.something(); }); to extract some data from a collection is reason enough. And there's also _.extend({}, something, somethingElse); to create a new object that has the functionality of both something and somethingElse. And so much more...

---------
- how to namespace with the IIFE and let jasmine see it?
- consider using input type=time, number; what versions are supported?
- JSLint

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use JSHint instead; not quite so crazy

@jplehmann
Copy link
Owner Author

@adamstrickland in moving code out to the HTML, there are two issues. First, the clear button is bound to the viewmodel to call init, which calls those two functions. So that dependency has to be decoupled.

Second, I am stru-gle-ling a little bit to successfully access my dependencies like KO through RequirejS because I am outside the "sandbox" I had setup with main.js. This may involve the order in which this code is executed relative to configuration in src/main.js. I'm sure I could figure this out after enough fiddling. It won't let me do an anonymous "define", and I'm not sure "require" is the right construct to use either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants