4
\$\begingroup\$

I'm looking for some advice in TDD for my implementation of the String Calculator kata described by Roy Osherov. Any advice regarding refactoring, structuring and polishing of the code is welcome. Still a novice at JS, so please go easy on me.

I have implemented the first 5 rules excluding the delimiter rule. I intend to do this kata everyday as a practice to get better.

The rules for the kata can be found here : http://osherove.com/tdd-kata-1/

Tests :

// StringCalculator-spec.js
describe("String Calculator",  function(){
    var calculator;


    beforeEach(function() {
      calculator = new StringCalculator();
    });

    it("should return zero when '' is passed", function(){
      expect(calculator.add('')).toEqual(0);
    });

    it("should return the number itself when a single number is passed", function(){
      expect(calculator.add('1')).toEqual(1);
    });

    it("should return the sum of the numbers if two are given", function(){
      expect(calculator.add('1,2')).toEqual(3);
    });

    it("should return the sum of an unknown amount of numbers", function(){
      // creates a random array
      randomArray = (length, max) => [...new Array(length)]
      .map(() => Math.round(Math.random() * max));
      // creates a random number between 1 and 100
      randy = Math.floor((Math.random() * 100) + 1);
      res = randomArray(randy,randy);
      // sums up an array
      sum = res.reduce((pv, cv) => pv+cv, 0);
      // converts the array to a string
      arg = res.join();
      expect(calculator.add(arg)).toEqual(sum);
    });

    it("should allow \\n in between the input number string", function(){
      expect(calculator.add("1\n2,3")).toEqual(6);
    });

    it("should not allow negative numbers", function(){
      expect( function(){ calculator.add("-1,2,3"); } ).toThrow(new Error("negatives not allowed"));
    });
  });

Code :

// StringCalculator.js
function StringCalculator(){
    }
    StringCalculator.prototype.add = function(string_numbers) {
      this.number = string_numbers;
      if (this.number.includes('-')) {
        throw new Error('negatives not allowed');
      }
      // replaces \n with ,
      numbers = this.number.replace(/(\r\n|\n|\r)/gm,",");
      if (numbers == '') {
        return 0;
      } else if(numbers.length === 1) { 
        return (parseInt(numbers));
      } else {
        res = numbers.split(',');
        var total = 0;
        var arrayLength = res.length;
          for (var i = 0; i < res.length; i++) {
            total = total + parseInt(res[i]);
          }
          return total;   
      }
    };
\$\endgroup\$
  • \$\begingroup\$ In the calculator, numbers is not declared with var,let, or const creating a global. \$\endgroup\$ – konijn Aug 16 '17 at 19:51
3
\$\begingroup\$
beforeEach(function() {
  calculator = new StringCalculator();
});

I usually recommend not using things like beforeEach (and similarly afterEach). The problem is that your tests are now implicitly dependent on an operation that's not referenced or even called anywhere in the test body. It's not immediately clear that a calculator was defined for each test. It's also not clear if it's a per test instance or a shared instance.

Tests should be independent and self-contained. The general rule I follow is I should be able to cut and paste a test to any part of the suite and still have it work, or remove it without affecting other tests. You can have shared utility functions that are called explicitly in the test body so that it's clear to the reader what happened, where something was obtained, etc.

function StringCalculator(){...}
StringCalculator.prototype.add = function(string_numbers) {...}

Although the rules mention "a method" which immediately would imply an instance and therefore classes, you don't necessarily need one. You'll only need a function that takes an input and gives an output. Keep it simple.

it("should return the sum of an unknown amount of numbers", function(){
  // creates a random array
  randomArray = (length, max) => [...new Array(length)]
  .map(() => Math.round(Math.random() * max));
  // creates a random number between 1 and 100
  randy = Math.floor((Math.random() * 100) + 1);
  res = randomArray(randy,randy);
  // sums up an array
  sum = res.reduce((pv, cv) => pv+cv, 0);
  // converts the array to a string
  arg = res.join();
  expect(calculator.add(arg)).toEqual(sum);
});

Never forget to use var, let or const if you don't want these variables to be defined in the global space (or at least the top level of the module). I usually recommend const whenever possible to ensure nothing has manipulated the variable since creation and let if you need to mutate it at some point (i.e. counters). Both are also block-scoped, eliminating guesswork and hoisting.

    var total = 0;
    var arrayLength = res.length;
      for (var i = 0; i < res.length; i++) {
        total = total + parseInt(res[i]);
      }
      return total; 

You can use array.reduce to accumulate values. It iterates through an array and uses the callback return value as first argument of the next call. An initial value can be supplied, otherwise iteration starts at the second value with first value as initial value.

Also, always check for NaN when converting strings to numbers. NaN passes through calculations, NaNing everything along the way. It's annoying to track the source of NaN and it may be very far from where it's spotted. Guarding near wherever it potentially comes up is a good practice.

Updating your logic:

function add(stringNumbers){
  const numbers = stringNumbers
    .replace(/(\r\n|\n|\r)/gm, ',') // Normalize delimiter
    .split(',')                     // Split values
    .map(n => parseInt(n, 10))      // Convert to integers

  // Check if numbers are ok. There's probably better APIs
  // to check if something in the array is invalid.
  if(numbers.some(n => Number.isNan(n))) throw new Error('Not a number')
  if(numbers.some(n => n < 0)) throw new Error('Negatives not allowed')

  // Add them all up
  return numbers.reduce((s, n) => s + n)

}
\$\endgroup\$
  • \$\begingroup\$ Any reason for using /(\r\n|\n|\r)/gm instead of just /\r?\n/g? \$\endgroup\$ – Gerrit0 Aug 17 '17 at 17:17
  • \$\begingroup\$ @Gerrit0 Nope. I just copy-pasted that portion. That can be refined. \$\endgroup\$ – Joseph Aug 17 '17 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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