2
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

Nice documentation. This code is clear and easy to read. Here are some basic nits:

  1. You should probably follow standard JSDoc conventions. Don't do this:

    /**
    *
    */
    

    Do this instead:

    /**
     *
     */
    

    If you're nitpicky about the @param, Float technically doesn't exist since all JavaScript numbers are floats. Use Number instead.

  2. 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);
    };
    
  3. 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 using typeof.

Happy coding!

\$\endgroup\$
  • \$\begingroup\$ Roger on the comments. I did === false to 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 the parseFloat, 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 the parseFloat and evaluate with != instead of !==. Thanks for the feedback \$\endgroup\$ – yBrodsky Jul 26 '17 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 Jul 26 '17 at 23:49

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.