I am on the process of changing jobs, so I would like to get an idea of what I do wrong in order to improve.
For this, I have created a small node module. It's very simple; it calculates linear motion (distance, velocity, time). I am interested in knowing what I do wrong, whether it's some crucial mistake or something secondary as the way I format my comments.
/**
* Checks whether two values are valid to be operated with
* @param {Float} operandA
* @param {Float} operandB
* @return {Boolean}
*/
const areValuesValid = (operandA, operandB) => {
if(isNaN(operandA) || isNaN(operandB)) return false;
if(operandA === null || operandB === null) return false;
return true;
}
/**
* Rounds a value to a max of two decimals
* @param {Float} val
* @return {Float}
*/
const round = (val) => {
return Math.round(val * 100) / 100;
}
/**
* Calculates the time in relation to the velocity and the distance
* @param {Float} velocity
* @param {Float} distance
* @return {Float}
*/
const calculateTime = (velocity, distance) => {
if(areValuesValid(velocity, distance) === false)
return 0;
if(parseFloat(velocity) === 0)
return 0;
return round(distance / velocity);
}
/**
* Calculates the velocity in relation to the time and the distance
* @param {Float} time
* @param {Float} distance
* @return {Float}
*/
const calculateVelocity = (time, distance) => {
if(areValuesValid(time, distance) === false)
return 0;
if(parseFloat(time) === 0)
return 0;
return round(distance / time);
}
/**
* Calculates the distance in relation to the velocity and the time
* @param {Float} velocity
* @param {Float} time
* @return {Float}
*/
const calculateDistance = (velocity, time) => {
if(areValuesValid(velocity, time) === false)
return 0;
return round(velocity * time);
}
module.exports = { areValuesValid, calculateTime, calculateVelocity, calculateDistance }
If anybody is willing to go the extra mile, I have created a repository with this code, which also includes some tests. Github. I would like to know if I go about the tests the right way, if the module structure makes sense and whatever else is criticizable.
1 Answer 1
Nice documentation. This code is clear and easy to read. Here are some basic nits:
You should probably follow standard JSDoc conventions. Don't do this:
/** * */Do this instead:
/** * */If you're nitpicky about the
@param,Floattechnically doesn't exist since all JavaScript numbers are floats. UseNumberinstead.See this SO post about rounding to 2 decimal places. Using it is completely up to you, there's nothing wrong with the way you're doing it. (This suggestion is slower, but looks cool I guess)
const round = val => { return +val.toFixed(2); };You know the constraints of your
areValuesValid()function, there's no need to=== false.const calculateTime = (velocity, distance) => { if (areValuesValid(velocity, distance) && parseFloat(velocity) !== 0) { return round(distance / velocity); } return 0; }Also, why the need to do
parseFloat(velocity)? If you know all your arguments will be numbers, this is unnecessary. If you don't know the data type of your arguments, you should probably enforce that they are numbers usingtypeof.
Happy coding!
-
\$\begingroup\$ Roger on the comments. I did
=== falseto avoid doing!areValuesValid()I was told that evaluating to false requires (at the processor level) the inversion of the truth tables, so it takes a little (insignificant nowadays) more time. As for theparseFloat, I was covering my ass in case somebody sends strings as params, which will work throughout the function except in that evaluation. I guess I could also remove theparseFloatand evaluate with!=instead of!==. Thanks for the feedback \$\endgroup\$yBrodsky– yBrodsky2017年07月26日 23:29:58 +00:00Commented Jul 26, 2017 at 23:29 -
1\$\begingroup\$ It depends on what you want this to be used for. If this is a user facing module/api that other people will be calling, then strong argument type checks are good. If this is going to be used internally in a project of your own, you can establish reasonable expectations for the parameters and their types. Also, I'd appreciate if you accepted this answer if it helped :) \$\endgroup\$omgimanerd– omgimanerd2017年07月26日 23:49:55 +00:00Commented Jul 26, 2017 at 23:49
You must log in to answer this question.
Explore related questions
See similar questions with these tags.