4
\$\begingroup\$

I recently applied for a job where the company gave me the following prompt for a code challenge. They have since told me "yeah, no thanks" but provided me with no feedback about why they came to that decision.

Would you be so kind as to review my code? You can even be harsh. I'd just like to know what this program is so severely lacking that it didn't warrant a phone call or an interview.

Prompt:

Generate markup HTML and CSS Add functionality to existing code
Turn in your work Provide feedback

Phase 1:

For this portion of the code challenge, you will develop code that renders an elevator panel that matches the provided mockup. You will then be adding JS code to make the elevator panel function according to the functional requirements below. You can control which way the elevator moves by selecting the “floor” buttons on the panel. As soon as a button is selected, the elevator will begin moving toward that floor.

UI Requirements:

A display panel that indicates the number of the current floor Green and red lights indicating whether the elevator is going up (green) or down (red) Four floor buttons

Four floors behind the elevator panel

Functional Requirements:

A floor can be selected from the elevator panel.

When a floor is selected, its button is highlighted.

Clicking on a floor button in the panel will cause the floors in the background to scroll up and down.

Multiple floors can be selected and will be executed in the order that they were selected.

The chosen button should remain highlighted until the floor is reached.

The current floor is shown on the display panel.

The elevator's current direction (up/down) is indicated by highlighting the corresponding green and red indicators in the display panel.

A visual cue is provided when you have arrived at the selected floor.

Selecting a button anchor does not navigate or change the URL.

Codepen

$(function() {

    var Elevator = function() {
      this.currentFloor = 1;
      this.targetFloor = null;
      this.targetButton = null;
      this.queue = [];
    }

    Elevator.prototype.move = function(indicator) {

      // indicate which direction elevator is traveling
      $(indicator).animate({opacity:1}, 500);

      // travel time should take 2 seconds per floor
      var travelTime = Math.abs(this.currentFloor - this.targetFloor) * 2000;

      // $.scrollTo() is cool
      $.scrollTo('#floor' + elevator.targetFloor, travelTime, 'easeInExpo');

      // Make changes to all relevant objects upon "arrival" time // var travelTime
      setTimeout(function() {

        // indicate elevator has stopped moving
        $(indicator).animate({opacity:0}, 500);

        // set the currentFloor to the targetFloor // "we've arrived!"
        this.currentFloor = this.targetFloor;

        // reflect current floor change in the DOM
        $('#floor-number').fadeOut(500, function() {
          $('#floor-number').html(this.currentFloor).fadeIn(500);  
        }.bind(this));

        // remove glow class from elevator button
        this.targetButton.removeClass('glow');

        // reset current target to null
        this.targetFloor = null;

        // re-apply click listener to 'that' button
        this.targetButton.on('click', elevator, addToElevatorQueue);

        // bind the elevator object to 'this'
      }.bind(this), travelTime)

    };



    var addToElevatorQueue = function(event) {

      var targetElevator = event.data;

      var button = $(this);

      // push the whole dom element into the queue
      targetElevator.queue.push(button);

      // add glow class to the button pushed
      button.addClass('glow');

      // remove click listener from 'this' button
      $(button).off('click')

    };


    var elevator = new Elevator();

    // pass elevator object to handler function 
    $('.floor-select-button').on('click', elevator, addToElevatorQueue);

    // initiate the elevator on the "bottom" / first floor
    $.scrollTo('#floor1', 500, 'easeInExpo');



    // 10 times a second, 
    // scan to see if there is a new destination 
    // the elvator can travel to
    setInterval(function() {

      // if there are any new destinations in the elevator.queue
      // and the elevator does not currently have a target it's moving to
      if (this.queue.length && this.targetFloor === null) {

        // splice off the 0th index of the this.queue
        this.targetButton = this.queue.splice(0,1)[0];

        // extract the floor number from that dom element and cast to type 'number'
        this.targetFloor = Number(this.targetButton.text());

        // if the target is "above" the current floor
        if (this.targetFloor > this.currentFloor) {

          // pass that indicator's css ID to Elevator.prototype.move()
          this.move('#up-indicator');

        // if the target is "below" the current floor
        } else {
          // pass the ID
          this.move('#down-indicator');          
        }

      }

    // bind globally initialized var elevator to 'this'
    }.bind(elevator), 100)

  })
\$\endgroup\$
3
\$\begingroup\$

In situations like this, you probably get about 30 seconds to make a good first impression. During that time, the two salient observations I had were:

  • Excessive comments: If you need to say everything twice, then your code isn't speaking for itself. Use comments to explain things that aren't obvious to someone who is reading the code. In particular, document function parameters, special values, and unusual conventions.

    Don't write clutter like this — the unnecessary comment just annoys the reader.

    // reset current target to null
    this.targetFloor = null;
    
  • Modelling: The challenge was probably meant as an exercise in writing model-view-controller code. Consider that there are panel buttons, a numeric floor display, and two arrows that all act as outputs. A loose coupling is called for, in which each of those UI elements can redraw their state based on the elevator state.

Here is an example of what I would be looking for.

var ElevatorController = (function() {
    /**
     * Constructor.  Takes an array of the names of the floors, from bottom
     * to top.
     */
    function ElevatorController(floors) {
        this.floors = floors.map(function(fl) { return fl.toString(); });
        this.travelTime = 1500;     // milliseconds from floor to floor
        this.currentIndex = 0;
        this.queue = [];
        this.callbacks = [];
      
        this.elevator = {
            motion: 0,              // -1 = Downward, 0 = Stopped; +1 = Upward
            currentFloor: this.floors[0]
        };
    }

    /**
     * Registers a function to be called when an event occurs.
     * Returns this object, for convenient chaining.
     *
     * The callback will be called with two parameters.  The first
     * parameter is the elevator state, which is an object with
     * attributes named "motion" (+1, 0, or -1) and "currentFloor".
     * The second parameter is a string describing the event type:
     * "up", "down", "arrived", or "floor".
     */
    ElevatorController.prototype.addCallback = function(callback) {
        this.callbacks.push(callback);
        return this;
    };

    ElevatorController.prototype.removeCallback = function(callback) {
        for (var i = this.callbacks.length - 1; i >= 0; i--) {
            if (this.callbacks[i] === callback) {
                this.callbacks.splice(i, 1);
            }
        }
        return this;
    };
  
    /**
     * Informs callbacks of the current state by sending them a "floor"
     * event.
     */
    ElevatorController.prototype.refresh = function() {
        fireEvent(this, 'floor');
        return this;
    };

    ElevatorController.prototype.press = function(floor) {
        var index = this.floors.indexOf(floor.toString());
        if (index < 0) return;      // Ignore invalid presses

        this.queue.push(index);
        react(this);
        return this;
    };

    function react(ctrl) {
        if (ctrl.queue.length == 0) return;

        if (ctrl.currentIndex == ctrl.queue[0]) {
            ctrl.elevator.motion = 0;
            ctrl.queue.shift();
            fireEvent(ctrl, "arrived");
            window.clearInterval(ctrl.interval);
            ctrl.interval = null;
        }
      
        if (ctrl.queue.length == 0) return;

        if (ctrl.elevator.motion) {
            ctrl.elevator.currentFloor = ctrl.floors[ctrl.currentIndex += ctrl.elevator.motion];
            fireEvent(ctrl, "floor");
            return;
        }
      
        if (ctrl.currentIndex < ctrl.queue[0]) {
            ctrl.elevator.motion = +1;
            fireEvent(ctrl, "up");
        } else if (ctrl.currentIndex > ctrl.queue[0]) {
            ctrl.elevator.motion = -1;
            fireEvent(ctrl, "down");
        }
        if (!ctrl.interval) {
            ctrl.interval = window.setInterval(function() { react(ctrl); }, ctrl.travelTime);
        }
    }

    function fireEvent(ctrl, event) {
        for (var i = 0; i < ctrl.callbacks.length; i++) {
            ctrl.callbacks[i](ctrl.elevator, event);
        }
    }

    return ElevatorController;
})();



var elev = new ElevatorController(['P2', 'P1', 1, 2, 3, 4]);
elev.addCallback(function debugReporting(ctrl, event) {
    console.log(event + " at " + ctrl.currentFloor);
}).addCallback(function buttonPanelCallback(ctrl, event) {
    if (event === "arrived") {
        $('button[value="' + ctrl.currentFloor + '"].lit').removeClass('lit');
    }
}).addCallback(function arrowLightCallback(ctrl, event) {
    if (event === "up") {
        $('#up-indicator').addClass('lit');
    } else if (event === "down") {
        $('#down-indicator').addClass('lit');
    } else if (event === "arrived") {
        $('#up-indicator').add('#down-indicator').removeClass('lit');
    }
}).addCallback(function floorNumberCallback(ctrl, event) {
    if (event === "floor") {
        $('#floor-number').text(ctrl.currentFloor);
    }
});

$(function init() {
    $('.panel button').click(function() {
        $(this).addClass('lit');
        elev.press($(this).val());
    });
    elev.refresh();
});
.display, .panel {
  width: 9em;
  text-align: center;
}
.display {
  margin: auto;
}
.display div {
  display: inline-block;
  margin: 0.2em;
}
.number {
  padding: 0.2em;
  width: 2em;
  text-align: right;
}
#up-indicator.lit {
  color: green;
}
#down-indicator.lit {
  color: red;
}
.panel {
  margin: auto;
}
.panel button {
  display: block;
  margin: auto;
  width: 3em;
}
.panel button.lit {
  color: yellow;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<div class="display">
  <div class="arrow" id="down-indicator">⬇︎</div>
  <div class="number" id="floor-number"></div>
  <div class="arrow" id="up-indicator">⬆︎</div>
</div>

<div class="panel">
  <button value="4">4</button>
  <button value="3">3</button>
  <button value="2">2</button>
  <button value="1">1</button>
  <button value="P1">P1</button>
  <button value="P2">P2</button>
</div>

\$\endgroup\$
  • \$\begingroup\$ The only thing I don't like here is that you're duplicating code. You've got your button/floor names in your markup, then you hard code them when you initialize your ElevatorController object. You should have been able to pull this information from the markup. \$\endgroup\$ – cimmanon Dec 31 '14 at 15:02
  • \$\begingroup\$ @cimmanon I was aware of that weakness. If I had to do something about it, I would do the opposite: query the controller for the floors, and insert the buttons into the DOM. \$\endgroup\$ – 200_success Dec 31 '14 at 18:07
  • \$\begingroup\$ @200_success I appreciate the note about pointless comments. When I personally read programs, I appreciate a line by line explanation like this. Some might call this a matter of preference, but I do agree about the "set this to null" comment, the code was very obvious. \$\endgroup\$ – James Dec 31 '14 at 23:42
1
\$\begingroup\$

I'm not a fan of your buttons being listed out of order (4-3-1-2). For someone else who has to read the markup, it might make them scratch their head, especially when the markup for the floors has them listed in the proper order.

<div id="controls">
  <button id="floor-4" class="floor-select-button">4</button>  
  <button id="floor-3" class="floor-select-button">3</button>  
  <button id="floor-2" class="floor-select-button">2</button>  
  <button id="floor-1" class="floor-select-button">1</button>  
</div>

CSS:

#controls {
  /* add prefixes as necessary */
  display: flex;
  flex-flow: wrap row-reverse;
}

See also: https://stackoverflow.com/questions/17883839/is-it-possible-to-flow-html-elements-up-using-css

\$\endgroup\$
  • \$\begingroup\$ Flex boxes are a bit too bleeding edge for my taste, at this point. Listing the buttons in descending order and floating them to the right would work more universally. \$\endgroup\$ – 200_success Dec 31 '14 at 18:12

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.