3
\$\begingroup\$

My task was to create a menu that displays three items (A,B,C), and has two nav buttons <- and ->. So for example, a menu of (A, B, C, D, E) starts off showing A,B,C and can press -> twice to display C,D,E.

My questions beyond general code review feedback are:

  1. How would you view the "programmer" behind this code in terms of ability/thought?
  2. How would you feel if you were working on legacy code and for whatever reason, had to edit a menu or add to a menu or debug this code?
  3. (Maybe overlap as the first question) - if you were a hiring manager on an interview, what kind of impression would this code give you?

I feel as though I have a ton to learn so I'm not expecting great reviews or anything, so your brutal honesty is appreciated.

Here are the flaws I noticed myself:

  • Img paths are hard-coded as ../images/misc/foo.png instead of of having the include path defined as a constant
  • The container with id=dataPreserve may seem unclear in its purpose/messy? This was my first time using jQuery's data function and I discovered that updating the entire container with .html();, also overwrote the data (!), and that I had to leave an outer-most container untouched during .html();s so as to preserve jQuery .data.

Here is the code:

HTML page

                    <div id="mbpCont">asdf
                    </div>
                    <script>
                        var uniqueName = 'mbp';
                        var subtitles = ['NAME 1', 'NAME 2', 'NAME 3', 'NAME 4', 'NAME 5'];
                        var icons = ['ph.png', 'ph2.png', 'ph3.png', 'ph4.png', 'ph.png'];
                        var descripts = ['Ut enim ad minim veniam, quis nostrud exercitation.', 'Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.', 'Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.', 'Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. ', 'final string'];
                        var links = ['prism.php', 'rp9.php', 'sa.php', 'ap.php', 'dd.php'];
                        var pos = 0;
                        var length = subtitles.length;
                        var isMoving = false;
                        mbpSlider = new Slider(uniqueName, subtitles, icons, descripts, links, pos, length, isMoving);
                    </script>

(sliders.js)

function Slider(uniqueName, subtitles, icons, descripts, links, pos, length, isMoving){
    var thisSlider = '#'+uniqueName+'Cont';
    $(thisSlider).html('<div class="sliderContainer" id="'+uniqueName+'">\
            <div class="sliderArrowContainer">\
                <a href="javascript:void(0);" id="matSliderLeft"><img class="sliderLeft" id="'+uniqueName+'Left" src="../images/misc/lArrowGrey.png"></a>\
            </div>\
            <div id="dataPreserve'+uniqueName+'" style="display:inline-block; *display:inline;zoom:1;">\
            <div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[0]+'"></div>\
                <div class="sliderSubtitle" id="'+uniqueName+'">'+subtitles[0]+'</div>\
                <div class="sliderText">'+descripts[0]+'</div>\
            </div>\
            <div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[1]+'"></div>\
                <div class="sliderSubtitle">'+subtitles[1]+'</div>\
                <div class="sliderText">'+descripts[1]+'</div>\
            </div>\
            <div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[2]+'"></div>\
                <div class="sliderSubtitle">'+subtitles[2]+'</div>\
                <div class="sliderText">'+descripts[2]+'</div>\
            </div>\
            </div>\
            <div class="sliderArrowContainer">\
                <a href="javascript:void(0);" id="matSliderRight"><img class="sliderRight" id="'+uniqueName+'Right" src="../images/misc/rArrowGreen.png"></a>\
            </div>\
        </div>');
    $('#'+uniqueName).data( { subtitles: subtitles, icons: icons, descripts: descripts, links: links, pos: pos, length:length, isMoving:isMoving } );

}

function formatId(eventTarget){
    if (eventTarget){
        var formatted = eventTarget.substr(0,3);
        return formatted;
    } else {return null;}
}

function canGoLeft(cur){
    var pos = getPos(cur);
    //alert('checking left: pos = '+pos);
    if (pos > 0) {
        return true;
    } else {
        return false;
    }
}

function canGoRight(cur){
    var pos = getPos(cur);
    var max = getMaxLength(cur);
    max = max - 3;
    if (pos < max) {
        return true;
    } else {
        return false;
    }
}

function getPos(cur){
    var pos = $('#'+cur).data('pos');
    return pos;
}

function getMaxLength(cur){
    var max = $('#'+cur).data('length');
    return max;
}

function isUpdating(cur){
    var moving = $('#'+cur).data('isMoving');

    if (moving){
        return true;
    } else {
        return false;
    }
}

function setMoving(cur, state){
    if (state){
        $('#'+cur).data('isMoving', true);
    } else {
        $('#'+cur).data('isMoving', false);
    }

}

function updateDisplay(cur, direction){
    if (updatePosition(cur, direction)){
        updateHtml(cur);
        setMoving(cur, false);
    } else {
        //alert('Error at updateDisplay');
        return null;
    } 
}

function updatePosition(cur, direction){
    var pos = getPos(cur);
    pos = pos+direction;
    //alert('new pos should be ' + pos);
    if (pos>=0){
        $('#'+cur).data('pos', pos);    
        return true;
    } else {
        return false;
    }
}

function updateHtml(cur){
    var thisSlider = '#dataPreserve'+cur;
    var pos = getPos(cur);

    updateArrows(cur);
    $(thisSlider).hide().html('<div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[pos]+'"></div>\
                <div class="sliderSubtitle" id="'+cur+'">'+subtitles[pos]+'</div>\
                <div class="sliderText">'+descripts[pos]+'</div>\
            </div>\
            <div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[pos+1]+'"></div>\
                <div class="sliderSubtitle">'+subtitles[pos+1]+'</div>\
                <div class="sliderText">'+descripts[pos+1]+'</div>\
            </div>\
            <div class="sliderBlock">\
                <div class="sliderIconHolder"><img class="sliderImg" src="../images/misc/'+icons[pos+2]+'"></div>\
                <div class="sliderSubtitle">'+subtitles[pos+2]+'</div>\
                <div class="sliderText">'+descripts[pos+2]+'</div>\
            </div>\
        ').fadeIn(600);
}

function getLArrowString(cur){
    var pos = getPos(cur);
    var str = '../images/misc/lArrowGrey.png';
    if (pos>0){
        str = '../images/misc/lArrowGreen.png';
    }
    return str;

}

function getRArrowString(cur){
    var pos = getPos(cur);
    var max = getMaxLength(cur);
    max = max - 3;
    var str = '../images/misc/rArrowGrey.png';
    if (pos<max){
        str = '../images/misc/rArrowGreen.png';
    }
    return str;
}

function updateArrows(cur){
    var lArrowId = '#'+cur+'Left';
    var rArrowId = '#'+cur+'Right';
    var lString = getLArrowString(cur);
    var rString = getRArrowString(cur);
    $(lArrowId).attr("src", lString);
    $(rArrowId).attr("src", rString);

}

/******************** ON CLICKS ************************/

$(".sliderLeft").live("click", function() {
    var cur = formatId(this.id);
    if(canGoLeft(cur)){
        if ( !isUpdating(cur) ){
            //alert('is not moving');
            setMoving(cur, true);
            updateDisplay(cur, -1);
        } else {
            //alert('is moving');
        }
    } else {
        //alert('cant go left');
    }
}); 

$(".sliderRight").live("click", function() {
    var cur = formatId(this.id);
    if(canGoRight(cur)){
        if ( !isUpdating(cur) ){
            //alert('is not moving');
            setMoving(cur, true);
            updateDisplay(cur, 1);
        } else {
            //alert('is moving');
        }
    } else {
        //alert('cant go right');
    }
}); 
\$\endgroup\$
5
\$\begingroup\$

Template

The first thing that comes into mind when building widgets is how the HTML is written. I'd avoid the HTML concatenated strings in JS. They're just so messy. I prefer templates instead, like Mustache.

Template in <script>

The parser will ignore since type is unknown to the browser:

<script type="text/template" id="myTemplate">
  <ul>
    {{#someArray}}
    <li>Current value: {{.}}</li>
    {{/someArray}}
  </ul>
</script>

Grab the HTML string:

var template = $('#myTemplate').html();

HTML string:

If you want self-contained, like for a plugin, design the HTML and compress to this:

var template = '<ul>{{#someArray}}<li>Current value: {{.}}</li>{{/someArray}}</ul>';

Semantics

You are creating a menu. In HTML terms, it's a list. Make it like so in static. Makes for good fallback when JS is disabled, or if something prevents the code from running, like lacking APIs or something:

<ul>
  <li>
    <img src="IMAGE_SOURCE" />
    <h4>TITLE_TEXT</h4>
    <p>ITEM_TEXT</p>
  </li>
  ...
</ul>
  • Additional data can be embedded as data-* attributes. Since this is per item, it would make more sense if it was placed in <li>

You can them morph it in JS to add the slider:

<div class="container">
  <div class="fixed-width-container">
    <ul>
      <li data-icon="ICON_PATH" data-something="SOME_MORE_DATA">
        <img src="IMAGE_SOURCE" />
        <h4>TITLE_TEXT</h4>
        <p>ITEM_TEXT</p>
      </li>
      ...
    </ul>
  </div>
  <a class="arrow-left">Left</a>
  <a class="arrow-right">Right</a>
</div>
  • .container would house the entire mechanism,
  • .fixed-width-container with overflow:hidden to hide the off-screen elements in the list.
  • The <ul> width would be the .fixed-width-container width multiplied by the number of items. This makes the slider expand beyond the container. If you need to display 3 items at a time, that would be (.fixed-width-container width / 3) * number of items.
  • The arrows positioned absolutely to the left and right of the container.
\$\endgroup\$
3
\$\begingroup\$

I guess my first thought is that I'm not sure why JavaScript is being used to generate the HTML, when the information, or view model, so to speak, is hard coded. It seems the HTML markup could all be written directly on the page.

If it is dynamic in nature, I think I would use an object or class for each image, which contains all its info, instead of individual arrays for each type of information.

Just first impressions. :)

\$\endgroup\$
  • \$\begingroup\$ For libraries that are "drop-in" and with minimal config, being able to be self-contained is a good attribute. Just add the library, a bit of config and the script does the HTML, CSS and JS. \$\endgroup\$ – Joseph Mar 13 '14 at 16:31

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.