4
\$\begingroup\$

I'm open to any feedback even if it's just a quick browsing over to get a quick opinion of my code.

As part of a job interview process I was given a coding challenge to complete. The challenge was pretty open and simply said to create a solution that graphed a linear equasion using javascript. Now that I've submitted my solution that I think is pretty well laid out, I wanted to see what kind of feedback I could get from the community about what I might be able to improve about my solution. (Readability, adhering to best practices, design patterns, etc.)

Here is a demo of the solution: https://codepen.io/adozark/full/jwwPNM/

In my solutions I have three files:

equation.js : this file contains a javascript class Equation that takes a string and parses, validates, and runs any linear equation (given everything past "y="). the function yGivenX(int) will output the y for any given x based on the linear equation.

function Equation () {

var equation;

    this.setEquation = function(value){
    equation = value;
}
this.getEquation = function(){
    return equation;
}

this.yGivenX = function (x){

    var eqx = equation;

    if(!this.validateEq()) return;

    //expand coeffecients : 2x -> 2*x
    eqx = eqx.replace(/([0-9])x/g,"$1*x");

    //fill in the value of x
    eqx = eqx.replace(/[xX]/g,x);

    var yCordinate = eval(eqx);

    return yCordinate;
}


this.validateEq = function (){

    var validEquationRegex =
    new RegExp('^\\s*(?:(?:(?:[0-9]+(?!\\.))|'
    +'(?:[0-9]+\\.[0-9]+))|(?:(?:[0-9]+(?!\\.))' 
    +'|(?:[0-9]*\\.[0-9]+))*[xX])(?:\\s*[*+\\/-]'
    +'\\s*(?:(?:(?:[0-9]+(?!\\.))|(?:[0-9]+\\.'
    +'[0-9]+))+|(?:(?:[0-9]+(?!\\.))|(?:[0-9]*'
    +'\\.[0-9]+))*[xX]))*\\s*$');

    var isValidEq = validEquationRegex.exec(equation);

    return isValidEq != null;
}

}// end of Equation

grid.js : this file contains the javascript class Grid, which when instantiated with a constructor taking an Equation and a html Canvas object, provides functions like graphEquation() which will draw the portion of the equation that passes through the html canvas and drawGridLines() which by default will mark every 50 pixels if the interval parameter is left undefined.

function Grid(canvas,equation,interval){

    var context = canvas.getContext('2d');

    var gridTicksInterval;

    if(!interval) gridTicksInterval = 50;

    this.DrawGridLines = function(){

        context.strokeStyle = 'black';

        //x line
        context.beginPath();
        context.moveTo(0,findAbsoluteY(0));
        context.lineTo(canvas.width,findAbsoluteY(0));
        context.stroke();

        //y line
        context.beginPath();
        context.moveTo(findAbsoluteX(0),0);
        context.lineTo(findAbsoluteX(0),canvas.height);
        context.stroke();

        context.textAlign="center";
        //y labels
        var offset = (canvas.height % gridTicksInterval)/2;

        for(var i = offset; i <= canvas.height ; i += gridTicksInterval){

            var width = context.measureText(i).width;
            context.fillStyle = 'white';
            context.fillRect(findAbsoluteX(-width/2),i-10,width,10);
            context.fillStyle = 'black';

            context.strokeStyle = 'black';
            context.fillText(findRelativeY(i),findAbsoluteX(0),i);
       }

       //x labels
       var xOffset = (canvas.width % gridTicksInterval)/2;

       for(var j = xOffset; j <= canvas.width ; j += gridTicksInterval ){

            //a center 0 label was already added by the y labels code.
            if(findRelativeX(j) === 0) continue;

            context.strokeStyle = 'black';
            context.textAlign="center";

            context.fillText(findRelativeX(j),j,findAbsoluteY(5));
      }

}
this.setGridTicksInterval = function(interval){

    gridTicksInterval = interval;
}

this.GraphEquation = function(){

    var xMin = -(canvas.width / 2);
    var xMax = -xMin;

    var yMin = equation.yGivenX(xMin);
    var yMax = equation.yGivenX(xMax);

    context.beginPath();
    context.moveTo(findAbsoluteX(xMin),findAbsoluteY(yMin));
    context.lineTo(findAbsoluteX(xMax),findAbsoluteY(yMax));

    context.strokeStyle = 'red';

    context.stroke();

}
//helper functions

function findAbsoluteX(x){

    return x + (canvas.width/2);
}
function findAbsoluteY(y){

    return -y + (canvas.height/2);
}
function findRelativeX(x){
    return x - (canvas.width/2);
}
function findRelativeY(y){
    return -y + (canvas.height/2);
}
}

index.html : this file is just an html file that includes some css, an input box and "Graph!" button, an html canvas, and the instantiation of my javascript classes/functions.

<html>
    <head>
        <style>
      .input{
        padding: 10px;
      }
      .message{
        height: 25px;
        color: red;
      }
      .center{
        text-align: center;
      }
      canvas {
      background-color: white;
      }
      body {
        margin: 0px;
        padding: 0px;
        background-color: white;

      }
        </style>
    </head>

    <body>     
        <div id='messageBanner' class='center message'>Enter an equasion to 
        graph!</div>
    <div class = "center input">
    y = <input type="text" id="functionTextBox">
    <button onclick="clicked()">Graph!</button>
    </div>

    <div class= "center">
    <canvas id="myCanvas" width="640" height="540" ></canvas>
    </div>

    <script src= "./equation.js"></script>
    <script src= "./grid.js"></script>

    <script>
      var canvas = document.getElementById('myCanvas');
      var context = canvas.getContext('2d');

      var messageBanner = document.getElementById('messageBanner');
      var functionTextBox = document.getElementById('functionTextBox');

      var myEq = new Equation();
      var myGrid = new Grid(canvas,myEq);

      //draw lines when page loads
      myGrid.DrawGridLines();

      function clicked(){

        myEq.setEquation(functionTextBox.value);

        //clear the canvas
        context.clearRect(0, 0, canvas.width, canvas.height);

        //re-draw grid lines.
        myGrid.DrawGridLines();

        if(!myEq.validateEq())
        {
          messageBanner.innerText =
          "The equasion must contain only real numbers"
          +" and the variable x. You can use multiplication, division,"
          + " addition, and subtraction.";
          +" Ex: 5x + 100 * 1.5x"
          return;
        }

          myGrid.GraphEquation();
          messageBanner.innerText = "Success!";

      }


    </script>
  </body>
</html>
\$\endgroup\$
2
\$\begingroup\$
  • Formatting is a little weird in some places. Specifically, the indentation in the JS files.
  • Eval leaves you open to injection attacks and should be used carefully. In this case there isn't much to exploit, but it's good practice to take certain measures.
  • Use strict mode in Equation.yGivenX(). This prevents eval'd code from changing things in the global scope.
  • It should be really clear what is being eval'd and using a long regex pattern in Equation.validateEq() sort of obfuscates it. Something like can this shorten your code and make it more readable:

    this.validateEq = function (){
      var allowedChars = "xX1234567890%^*()-+/. ";
      for (var i = 0; i < equation.length; i++)
        if (allowedChars.indexOf(equation.charAt(i)) <= 0) return false;
      return true;
    };
    
  • Best practice is to use semicolons after a statement, ie, after your method definitions. JS will insert them if you leave them out but it's worth it to add them just so your IDE doesn't show the warning.

  • At the end of Equation.yGivenX() you are assigning the result to a variable and then immediately returning it. You are unnecessarily committing it to memory. Instead, just return eval(eqx);

  • In Equation.yGivenX() your second regex accommodates either a lower or upper case x but the first one only checks for a lower case. Thus, 9x-9 and 9X-9 will produce different results. Change your first regex to: eqx = eqx.replace(/([0-9])[xX]/g, "$1*x");
  • Good practice to separate your JS and CSS from your markup. I would put them in separate files. Similarly, you should favor attaching events with JS over using an onclick attribute in the HTML.
  • To improve UX, wrap the textbox in a form and fire clicked() onsubmit rather than on button press. This will allow users to hit the enter key instead of using the mouse.
  • You're missing title and doctype in your HTML.

This is pretty cool. Here's a plunk with some of my suggestions implemented: https://plnkr.co/edit/mpt6SYR29s5u6JZhNiWd?p=preview

\$\endgroup\$

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.