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 1 down vote favorite
1

Below is a basic filter I created with Angular that does temperature conversion. I'd like to get thoughts on how to improve this code from both an Angular perspective and a general JS perspective.

The filter should take 2 arguments:

scale: The temperature scale to convert to

label: Whether to append a degree symbol to the number

In the view:

// should output: 99
{{ 37 | formatTemperature: 'F' }}

// should output: 99°
{{ 37 | formatTemperature: 'F':true }}

// should output: 37
{{ 98.6 | formatTemperature: 'C' }}

The filter:

(function() {
  'use strict';

  angular.module('example')
    .filter('formatTemperature', [
      function() {
        return function(input, scale, label) {
          var value = parseInt(input, 10),
              convertedValue;

          if (isNaN(value)) throw new Error('Input is not a number');

          if (scale === 'F') {
            convertedValue = Math.round(value * 9.0 / 5.0 + 32);
          } else if (scale === 'C') {
            convertedValue = Math.round((value - 32) * 5.0 / 9.0);
          } else {
            throw new Error('Not a valid scale');
          }

          return label ? convertedValue += '\u00B0' : convertedValue;
        };
      }
    ]);
})();

Testing the filter

(function() {
  'use strict';

  describe('formatTemperature test', function() {
    beforeEach(module('example'));

    var filter;

    beforeEach(inject(function(_$filter_) {
      filter = _$filter_('formatTemperature');
    }));

    it('should convert temperature from Celsius to Fahrenheit', function() {
      expect(filter(37, 'F')).toEqual(99);
    });

    it('should convert temperature from Fahrenheit to Celsius', function() {
      expect(filter(98.6, 'C')).toEqual(37);
    });

    it('should append a % to the converted value', function() {
      expect(filter(37, 'F', true)).toEqual('99\u00B0');
    });

    it('should throw an error if the scale is invalid', function() {
      expect(function() {
        filter(37, 'G');
      }).toThrow(new Error('Not a valid scale'));
    });

    it('should throw an error if value is not a number', function() {
      expect(function() {
        filter('test', 'F');
      }).toThrow(new Error('Input is not a number'));
    });
  });

})();
share|improve this question
    
So what happens if I have a temperature given in Kelvin? –  ferada Jun 17 at 18:05
    
You might have a look at github.com/gentooboontoo/js-quantities ;) –  glepretre Jun 19 at 6:57

1 Answer 1

up vote 2 down vote

On the principal that functions should do one thing and do it well, consider breaking out some pieces of your filter into smaller functions for readability.

Filter

(function () {
    'use strict';

    angular.module('example')
        .filter('formatTemperature', formatTemperatureFilter);

    var degreesSymbol = '\u00B0';

    function convertCelsiusToFahrenheit(value) {
        return Math.round(value * 9.0 / 5.0 + 32);
    }

    function convertFahrenheitToCelsius(value) {
        return Math.round((value - 32) * 5.0 / 9.0);
    }

    function addDegreesSymbol(value) {
        return value += degreesSymbol;
    }

    function formatTemperatureFilter() {
        return function (input, scale, label) {
            var value = parseInt(input, 10),
                convertedValue;

            if (isNaN(value)) throw new Error('Input is not a number');

            if (scale === 'F') {
                convertedValue = convertCelsiusToFahrenheit(value);
            } else if (scale === 'C') {
                convertedValue = convertFahrenheitToCelsius(value);
            } else {
                throw new Error('Not a valid scale');
            }

            return label ? addDegreesSymbol(convertedValue) : convertedValue;
        };
    }

})();

Make sure that your tests are testing one thing!

This will fail if you changed your conversion functions. Maybe you want to add 1 decimal place in the future.

it('should append a % to the converted value', function() {
      expect(filter(37, 'F', true)).toEqual('99\u00B0');
});

Change to a matcher that looks for the degrees symbol

it('should append a degrees symbol to the converted value', function() {
     expect(filter(37, 'F', true)).toContain('\u00B0');
});

Lastly, the only acceptable values for the scale are F and C, should this be case sensitive?

share|improve this answer

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.