current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
1

To learn the language of JavaScript, I did a dojo exercise by Roy Osherove. Is there a better way to organize this so that the only method exported is add while keeping it in one single module?

module.exports = {
  add: function (string) {
    this._checkForError(string);
    this._checkForNegatives(string);
    return this._result(string);
  },

  _result: function (string) {
    var numbers = this._numbers(string);
    var result = numbers.reduce(function (sum, number) {
      return sum + number;
    });
    return result;
  },

  _numbers: function (string) {
    var modded = this._modded(string);
    var numbers = modded.map(function (number) {
      return parseInt(number, 10) || 0;
    });
    return numbers;
  },

  _checkForError: function (string) {
    if (string.match(/\n$/)) {
      throw 'Expression ending in newline!';
    }
  },

  _negatives: function (string) {
    var numbers = this._numbers (string);
    var negatives = numbers.filter(function (number) {
      return number < 0;
    });
    return negatives;
  },

  _delimiter: function (string) {
    return (string.substr(0, 2) === '//') ? string[2] : ',';
  },

  _checkForNegatives: function (string) {
    var negatives = this._negatives(string);
    if (negatives.length > 0) {
      throw 'Negatives not allowed: ' + negatives.join(', ');
    }
  },

  _modded: function (string) {
    var delimiter = this._delimiter(string);
    return string.replace(/\n/, delimiter)
      .split(delimiter);
  }
};
share|improve this question
    
Yes, don't export it. –  Dagg Jun 2 at 3:26
    
I see what you're saying, would you mind illustrating a simple example? @Dagg –  Lucien Lachance Jun 2 at 3:27
    
Just use a regular function declaration that isn't assigned to a property of the exports object. –  Dagg Jun 2 at 3:28
    
After spending all night reading on modules, I realized that the old module pattern does the exact same thing as module.exports. It took me awhile to get my head around that. In this case, I can wrap the module in a function and at the end attach a return to the function add() and define the remaining methods above to exclude it from being "exposed". @Dagg –  Lucien Lachance Jun 2 at 15:20
add comment

1 Answer

up vote 2 down vote accepted

module.exports is just an object. Set properties on it that you want to expose.

exports.add = add

This is also equivalent in this case.

module.exports = {add: add}

Now, when you require this module, you'll get an object with one property: add.

There are a few other small issues:

Nothing about your design depends on accessing methods via this. This module is clearer without bringing this into the mix.

function add (string) {
  _checkForError(string)
  _checkForNegatives(string)
  return _result(string)
}

This code is quite easy to follow already, but It would be even better if you had your methods in the order they are called so the code reads like an article: broad info at the top, specific details later on.

I'd also leave out semicolons where they will be handled by automatic semicolon insertion, but that's a style choice.

function _checkForError (string) {
  if (string.match(/\n$/)) {
    throw 'Expression ending in newline!'
  }
}

function _checkForNegatives (string) {
  var negatives = _negatives(string)
  if (negatives.length > 0) {
    throw 'Negatives not allowed: ' + negatives.join(', ')
  }
}

function _negatives (string) {
  var numbers = _numbers(string)
  var negatives = numbers.filter(function (number) {
    return number < 0
  })
  return negatives
}

function _result (string) {
  var numbers = _numbers(string)
  var result = numbers.reduce(function (sum, number) {
    return sum + number
  })
  return result
}

function _numbers (string) {
  var modded = _modded(string)
  var numbers = modded.map(function (number) {
    return parseInt(number, 10) || 0
  })
  return numbers
}

function _modded (string) {
  var delimiter = _delimiter(string)
  return string.replace(/\n/, delimiter).split(delimiter)
}

function _delimiter (string) {
  return (string.substr(0, 2) === '//') ? string[2] : ','
}
share|improve this answer
    
Thanks for explaining this, I appreciate it. –  Lucien Lachance Jun 2 at 21:42
    
glad to be helpful! –  nrw Jun 2 at 22:32
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.