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

tools.parseUnitString and tools.parseAngle #167

Merged
merged 8 commits into from Dec 6, 2012
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 48 additions & 1 deletion src/tools.js
Expand Up @@ -6,7 +6,8 @@ define([], function() {
noop = function() {},
push = [].push,
slice = [].slice,
toString = {}.toString;
toString = {}.toString,
PI = Math.PI;

/**
*
Expand Down Expand Up @@ -339,6 +340,52 @@ define([], function() {
array.length -= numRemoved;

return array;
},

/**
* Parse a given unit string into the amount and unit
*
* @param {any} any Anything that can be converted to a string. E.g. '7deg', ['50%']
* @returns {String} unit
*/
extractUnit: function (any) {
if (typeof any === 'undefined' || typeof any === 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we need that check. What about "", null, [], {}, …? I see your point but I don't think that we should expect undefined and number of all types. The cast in L357 takes care of invalid values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so do some test after var unit = ... to make sure we return the empty string if given a poor argument?

String(undefined) //=> "undefined"
String(null) //=> "null"
String({}) //=> "[object Object]"
String([]) //=> ''
String(['singleDepth']) //=> 'singleDepth'
String(['bigger', 'array', 4]) //=> 'bigger,array,4'

Copy link
Contributor

Choose a reason for hiding this comment

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

No. That's sth the author/dev need to fix. That's why I suggested to remove the type-checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I can roll with that. Removed the checks, tests are still passing.

return '';
}

// make sure it's a string and remove trailing whitespace
var unit = String(any).replace(/\s+$/, '');
// returns extracted unit or empty string
return unit.match(/[a-z%]*$/i)[0];
},

/**
* Parses the angle string to radians
* https://developer.mozilla.org/en-US/docs/CSS/angle
*
* @param {Number|String} angle The angle/unit string
* @returns {Number} The angle in radians
*/
parseAngle: function(angle) {
var radians,
unit = tools.extractUnit(angle),
amount = parseFloat(angle);

switch (unit) {
case '': // default
case 'rad':
radians = amount; break;
case 'deg':
radians = amount * PI / 180; break;
case 'grad':
radians = amount * PI / 200; break;
case 'turn':
radians = amount * 2 * PI; break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need "default:" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don’t all switch statements need a default? I don’t often use switch statements. I usually do the dirty super ternary instead :|

var radians = 'rad' === unit ? amount
            : 'deg' === unit ? amount * PI / 180
            : 'grad' === unit ? amount * PI / 200
            : 'turn' === unit ? amount * 2 * PI
            : 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I think a switch statement don't need necessarily a "default", especially when the default value was previously set (in L470). You already implicitly assigned undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of reversing it? Rather than case '' falling through to case 'rad' we let 'rad' fail through to default.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we wouldn't even need case 'rad'. But parseAngle('2whatever') would return 2 (rad). :(

radians = 0;
}

return radians;
}
};

Expand Down
94 changes: 94 additions & 0 deletions test/tools-spec.js
Expand Up @@ -284,4 +284,98 @@ define([
});
});

/*--------------------------------------------------------------------------*/

describe('tools.extractUnit', function() {
var extractUnit = tools.extractUnit;

it('return the unit from a unit string', function() {
var returned = extractUnit('9deg');
var expected = 'deg';

expect(returned).toEqual(expected);
});

it('should work with negative numbers', function() {
var returned = extractUnit('-741232grad');
var expected = 'grad';

expect(returned).toEqual(expected);
});

it('should work with trailing whitespace', function() {
var returned = extractUnit('-741232grad ');
var expected = 'grad';

expect(returned).toEqual(expected);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Other edge cases:

  • should work with leading whitespace
  • should return an empty string in case of a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests for both of those as well as the unit being % and for when undefined is passed in.

it('should work with leading whitespace', function() {
var returned = extractUnit('-741232 grad ');
var expected = 'grad';

expect(returned).toEqual(expected);
});

it('should return an empty string in case of a number', function () {
var returned = extractUnit(7531);
var expected = '';

expect(returned).toEqual(expected);
});

it('should return the string when given an array', function () {
var returned = extractUnit(['50s']);
var expected = 's';

expect(returned).toEqual(expected);
});

it('should work with percentages', function () {
var returned = extractUnit('50%');
var expected = '%';

expect(returned).toEqual(expected);
});
});

describe('tools.parseAngle', function() {
var parseAngle = tools.parseAngle;

it('calculates radians for degrees', function() {
var calculated = parseAngle('45deg');
var expected = 0.7853981633974483;

expect(calculated).toBeCloseTo(expected, 10);
});

it('calculates radians for turns', function() {
var calculated = parseAngle('1turn');
var expected = 6.283185307179586;

expect(calculated).toBeCloseTo(expected, 10);
});

it('calculates radians for gradians', function () {
var calculated = parseAngle('-32grad');
var expected = -0.5026548245743669;

expect(calculated).toBeCloseTo(expected, 10);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Other edge cases:

  • calculates radians for radians as a string
  • calculates radians for radians as a number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for radians as a string as in parseAngle('0.321512324') or as in parseAngle('0.321512341rad')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests for the following scenarios:

parseAngle('0.42131') parseAngle(0.42131) and parseAngle('0.42131rad')

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

it('calculates radians for radians (string)', function () {
var calculated = parseAngle('-0.5026548245743669rad');
var expected = -0.5026548245743669;

expect(calculated).toBeCloseTo(expected, 10);
});

it('calculates radians for radians', function () {
var calculated = parseAngle('-0.5026548245743669');
var expected = -0.5026548245743669;

expect(calculated).toBeCloseTo(expected, 10);
});
});

});