2
\$\begingroup\$

I've written a script that I would love to get feedback evaluating the following: Module Pattern implementation, robustness, conciseness, cross-browser compatibility. My goal is to eventually use this script and others, as part of my portfolio for a job application. All criticisms are most welcome, especially if you're already a professional JS developer.

var tashapp=(function(){

      var $=document.getElementById.bind(document);
     //return appropriate event listener based on browser
      var setListener = function(){

          if (document.addEventListener) {

          return function(el, evnt, fn){
            el.addEventListener(evnt,fn,false);
          }

        }else{

          return function(el,evnt,fn){
              el.attachEvent('on'+evnt,fn,false);  
        }
      };

      }();

    //Select correct img size for background
      var setDivBg = function(){

          var divs=document.getElementsByClassName('fullBg');     
          var widths=['800','1024','1200','1280','1400','1440','1600','1680','1920','2400','2500','2600'];     
          var elem, bgImg, elWidth, elHeight, count=0, imagePath='images/';

          for (i=0; i < divs.length; i++) 
            {            
              /* find dimensions of each div and select, the appropriate image path for bg*/
               elem=divs[i];
               elWidth=elem.offsetWidth;
               elHeight=elem.offsetHeight; 

              for(var i=0; i < widths.length; i++){
                    //find closest width
                    if(elWidth < widths[i]){                             
                        continue;
                    } else{

                      elem.id=='main'? elem.style.backgroundImage="url('"+imagePath+widths[i]+"/delicious_steak.jpg')" : elem.style.backgroundImage="url('"+imagePath+widths[i]+"/tableset.jpeg')";
                    }
              }                 

            }

        }; //*end setDivBg


      var setNavPos=function (){

          var nav=$('navlinks');
          var navWidth=nav.offsetWidth;
          var navHeight=nav.offsetHeight;

          var navPos=(window.innerWidth-navWidth)/2;
          nav.style.left=navPos+'px';

      }; //*End setPos   

     var setAboutText=function(){

          var divs=document.getElementsByClassName('aboutText');
          var i, p,max;
          var h=[];
          //set uniform position for every aboutText div
          for(i=0; i < divs.length; i++){                   
               p=divs[i].parentElement;
               divs[i].style.width=(p.offsetWidth)*0.95+'px';
               divs[i].style.height='auto';

               h.push(parseInt(divs[i].offsetHeight)); 

          }
          /*Find which aboutText div is tallest. set others to same*/
            max=Math.max.apply(Math,h);

          for(i=0; i < divs.length; i++){

                divs[i].style.height=max+'px';
          }

    };

Utility callbacks

     //function to handle creating and appending elements. 

    var appendToDoc=function(par,elType,attr,attrName){

          var el=document.createElement(elType);
          el.setAttribute(attr,attrName);

          return par.appendChild(el);

    };


    var removeEl=function(){
            var el=this, holder=$('menuholder');

            fadeToBlack(el.parentElement);

           setTimeout(holder.parentElement.removeChild(holder), 2000);              

    }

     //kills loaded menu by transitioning to invisible then removing element.  
    var fadeToBlack=function(el){
        //change elements opacity to 0;            
        el.className='invisible';

    }

End utility callbacks

    //Implement google Map

    var mapIt=function(){
            var latLong ={lat:51.38650,lng:1.38126 };
            var mapCanvas = $('map');
            var mapOptions = {
                            center: new google.maps.LatLng(51.38650, 1.38126),
                            zoom: 13,
                            mapTypeId: google.maps.MapTypeId.ROADMAP
                          };

            var map = new google.maps.Map(mapCanvas, mapOptions);
            var marker = new google.maps.Marker({
                 position:latLong,
                 map:map,
                 title:'Tashi\'s Pantry!',
                 });
          };

   //function to create and view Menus
    var viewMenu=function(){

        var docBody=document.body;
        var frame,frameDoc,exit;            
        appendToDoc(docBody, 'div', 'id', 'menuholder');
        appendToDoc($('menuholder'),'div','id','container');
        appendToDoc($('container'),'iframe','id','currMenu');

         frame=$('currMenu');
         console.log(frame);
         frame.src=this.id+'.php';

         frame.onload=function(){                
            frameDoc=frame.contentWindow.document;
            setListener(frameDoc.getElementById('exit'),'click',removeEl);
         };



    };//**End view menu. 

    var resizeFn=function(){
        //set text position, set aboutText div size, reset Image sizes
        setDivBg();
        setAboutText();
        setNavPos();
    };  

    return {

            start: function(){

                     var menuLinks=document.getElementsByClassName('showmenu');
                     var i;

                    document.onreadystatechange=function(){
                    if (document.readyState=='loading' || document.readyState=='interactive'){console.log('loading and setting menu header');
                    } else{

                      //initialize backgrounds and text, text positions etc
                        setDivBg();
                        setAboutText();
                        setNavPos();

                      //add event Listeners 
                      setListener(window, 'resize', resizeFn);

                      for(i=0; i < menuLinks.length; i++){

                          setListener(menuLinks[i], 'click', viewMenu);


                      }

                    }
                }; 

            },

          map:function(){mapIt()},  



      } //**end return object 

})();

tashapp.start();
tashapp.map();
\$\endgroup\$
1
+50
\$\begingroup\$

Module Pattern

The module pattern is perfect if you write a library. There are more professional approaches like AMD or CommonJS, but they are most useful when you write a big application.

For a small app like yours the module pattern can be useful; but keep in mind that there is no reason to avoid using other libraries then: I think your code would be much clearer if you used jQuery.

(There is also yet another way to work with modules: ECMAScript 6. You can use babel and browserify to compile it to ES5. Depending on your (hopefully future) job you might have to use it.)

Robustness

getElementById() returns null if the element couldn't be found. If your HTML doesn't fit your code you will get a lot of type errors.

EDIT: You also depend on google.maps. You need to, but it should be more clear.

var tashapp = (function(google.maps) {
    ..your code..
})(google.maps);

Conciseness

First of all, you really messed up with indentation! If you decide to use 4 spaces, use them through the hole of your code (an IDE will help you with that). And don't use so many new lines. They make reading the code harder (see removeEl()).

The JS coding convention differs from your style. I would recommend you follow at least these rules:

  • one space after each comma
  • use // for one line comments
  • surround = by spaces

Most important about a code style is that you are consistent. So even if you decide not to follow these rules, at least be consistent with it!

Cross-browser

In the third line you use bind(). This function is really useful, but unfortunately it wasn't implemented before IE 9. Same applies to getElementByClassName() This means you don't support IE 8. Maybe you don't care, but if you do I really urge you to use jQuery. You get support for IE 6, 7 and 8 (if you use jquery-1.*) and it will improve your code if you use it wisely.

PS: MDN is your friend!

Some other observations

Exact the same code you wrote in resizeFn you use in the start method of your return object.

function(){mapIt()} makes no sense. You can simply write mapIt (functions are values too!).

//**End view menu. isn't good style: If you use it, it either means you are using a bad editor with no support for matching bracket highlighting or the function is too long. Avoid it.

You refer to some images. If you would like to change the names of these images, where would you guess is the hard-coded string in the code? setDivBg? This is one of the many locations where I would use a utility function.

In setDivBg you use i in two nested for loops. Is that intended? You know that var i doesn't introduce a new variable with block scope?

Utility functions make sense when they help you to avoid duplicating code or structure your app. fadeToBlack however is only used once and contains one line.

General advice

You used direct DOM manipulation. It is great that you now how to do that, but most JS developer work with frameworks today. I am not saying that this is the only way to make an app. But make sure you have code in your portfolio showing that you know at least two of jQuery, React, AngularJS (and maybe Ember, Backbone, Vue, Handlebars, etc.).

Use a package manager like npm or bower.

Write unit tests. You should write some for utility functions (using Jasmine for example), but if you want to make a really good impression you should think about Phantom (but honestly I don't know anybody using it).

Write "use strict"; in the second line. You will probably get a lot of errors but you will also avoid making mistakes unknowingly.

Never use ==. It will convert types in a most unobvious way. Use === instead (it won't break your code here).

Buy yourself a copy of "Effective JavaScript" by David Herman. It will teach you some good practices, that really come in handy sometimes.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ +1 for lots of great advice. Only thing I would add is that there's a time and place for == over ===. The key is to know which to use and when. \$\endgroup\$ – OffTheBricks Mar 14 '16 at 22:31
  • \$\begingroup\$ Thanks so much for such a complete response. Im using the module pattern so I can demonstrate a level of proficiency with it in an interview. For example, this is from a recent posting on Indeed.com : "Strong JavaScript skills, including asynchronous programming and object-oriented design patterns". Hence my post. I do agree with you about most companies using libraries though, and I'm gaining proficiency there too. i'm gonna wait for a couple more responses, but you're clearly in lead position for the bounty right now :-) \$\endgroup\$ – steveBK Mar 16 '16 at 13:45
  • \$\begingroup\$ @steveBK: Object-orientated design usually refers to ES6 classes or the prototype chain. Module pattern has much more to do with encapsulation. \$\endgroup\$ – Anton Lorenzen Mar 16 '16 at 22:41
1
\$\begingroup\$

Use Node, with webpack or browserify to organize modules if you are working on a portfolio project. There isn't really a need to reimplement the module pattern. If you work with javascript professionally it is very likely you will be using npm, and all npm modules that you install will use the commonjs module specification.

See also http://wiki.commonjs.org/wiki/Modules/1.1

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks for the input, its great to have details about how/what tools are being used in the professional environment. the goal is to be able to demonstrate understanding of the design pattern without using a library. I've seen a lot of postings asking for that. but I am working on those other libraries too. \$\endgroup\$ – steveBK Mar 16 '16 at 13:48

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.