1
\$\begingroup\$

I have a set of images with class = tt like so:

<img src="images/arc_unselected.png" title="Arc" class="tt" />

and I have written this code to change the images after clicking:

     $(".tt").click(function(event) {                 
           var title = event.target.getAttribute("title")        
           resetOtherImages(title)       
           if( title == "info"){
              if ( event.target.getAttribute("src") == "images/information_selected.png"){                   
                 $(this).attr('src', 'images/information_unselected.png')                    
                 }
              else {                          
                  $(this).attr('src', 'images/information_selected.png')
                 }                   
           }else if( title == "Up"){
                 if ( event.target.getAttribute("src") == "images/up_selected.png"){                     
                 $(this).attr('src', 'images/up_unselected.png')                     
                 }
              else {                          
                  $(this).attr('src', 'images/up_selected.png')
                 }                                 
           }else if( title == "Down"){
                 if ( event.target.getAttribute("src") == "images/down_selected.png"){                   
                 $(this).attr('src', 'images/down_unselected.png')                   
                 }
              else {                          
                  $(this).attr('src', 'images/down_selected.png')
                 }                                 

           }else if( title == "SE"){
                 if ( event.target.getAttribute("src") == "images/se_selected.png"){                     
                 $(this).attr('src', 'images/se_unselected.png')                     
                 }
              else {                          
                  $(this).attr('src', 'images/se_selected.png')
                 }                                             
           }else if( title == "Arc"){
                 if ( event.target.getAttribute("src") == "images/ar_selected.png"){                     
                 $(this).attr('src', 'images/arc_unselected.png')                    
                 }
              else {                          
                  $(this).attr('src', 'images/arc_selected.png')
                 }                                             
           }               
        })  

        function resetOtherImages(title) {
        $('.tt').each(function(){
            if($(this).attr('src') !== title)
            {
              var newName = changeName($(this).attr('src'))
              $(this).attr('src',newName)   
            }
        });
        }

        function changeName(imgName) {    
           var pos = imgName.indexOf("_");
           var first = imgName.substring(0,pos);
           return first.concat("","_unselected.png");
        }

It works without any problem, but since I have not much experience with javascript I wanted to know if there is a better way to do this, more compact and more readable.

\$\endgroup\$
  • \$\begingroup\$ I don't understand your comparison " if($(this).attr('src') !== title) " . In your example : " 'images/arc_unselected.png' !== 'Arc' " . If I guess it right your comparison would be true every time and then execute the code within. So what's the meaning of that ? Or do I get it wrong? Then please correct me. \$\endgroup\$ – michael.zech Jul 2 '16 at 7:31
3
\$\begingroup\$

Interesting question, in that you gave away most of the answer in this function:

    function changeName(imgName) {    
       var pos = imgName.indexOf("_");
       var first = imgName.substring(0,pos);
       return first.concat("","_unselected.png");
    }

You know that all unselected images end with "_unselected.png" and by extension you know that all selected images end with "_selected.png". As a side note I would not have used indexOf, and substring, I would have used a split and string concatenation thru +:

    function deriveUnselectedVersion( imagePath ){    
       var firstPart = imagePath.split("_")[0];
       return firstPart + '_unselected.png';
    }

I am not 100% confident about deriveUnselectedVersion but it sure sounds better (and is more meaningful) than changeName.

Anyway, this is how I would write the click handler:

 $(".tt").click(function(event) {                 
       var image = event.target,
           title = image.getAttribute("title"),
           sourceInfo = image.getAttribute("src").split("_"),
           base = sourceInfo[0],
           version = sourceInfo[1];

       resetOtherImages(title);     

       if( version == "selected.png" ){
         $(this).attr('src', base + "_unselected.png");      
       } else {
         $(this).attr('src', base + "_selected.png");            
       }

       //Or we could go really (too) fancy
        $(this).attr('src', base + ( version == "selected.png" ? "_unselected.png" : "_selected.png" ) );

    });

You will notice:

  • No missing semi colons at the end of statements
  • 1 var statement on top to rule them all
  • Only 1 $() statement executed (these are expensive)
  • This will work for all new buttons as long they follow the same naming scheme
  • Much shorter
  • I am still ambivalent about using var title, since title is used in only one place..

A final advice is that writing code follows the same rules as writing prose, dont repeat yourself (too much) aka DRY.

    function resetOtherImages(title) {
    $('.tt').each(function(){
        if($(this).attr('src') !== title)
        {
          var newName = changeName($(this).attr('src'))
          $(this).attr('src',newName)   
        }
    });
    }

See how many times you repeat $(this).attr('src') ? You can and should do better than that.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

Well, I think you should consider if there exists any possibility of adding more rules. If that's the case open/close principle states that your code should be closed to modification and open for extension. Now if you try to add another case you will endup modifying code existing code (adding code without modifying existing one), to make it open you should provide a way to extend it. Something like a collection of rules and actions could work.

Something like this for each rule:

var firstRule = {
  match: function(title){
    return title === "info";
  }
  act: function(img){
    //apply change image here
  } 
}

After having al rules/actions with a structure of of match/act you can apply polymorphism.

For instance you can have an array of rules.

var rules = [];
rules.add(firstRule);
rules.add(/*... more rules*/);

Finally you can iterate in order to see if the rule applies and then act depending on it.

Each time you need to add a new rule, you will be creating a new object with both function match and act functions, this way you will not modify code, you will be adding code.

Also, you should take a look at repeated code... There is like pattern inside each case, the only thing it changes is the prefix on image names. If you are sure that the images will allways be named as "image/{title}_unselected.png" and "image/{title}_selected.png" you take this out into a separate funcion.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ I just could not bring myself to fixfistRule, made me smile ;) \$\endgroup\$ – konijn Jul 1 '16 at 18:06
  • \$\begingroup\$ @konijn that was really funny, made me laugh too. Got it fixed. \$\endgroup\$ – bruno.bologna Jul 1 '16 at 18:23
0
\$\begingroup\$

I think your code will be easier to read if you use a switch statement and factor event.target.getAttribute("src")

$(".tt").click(function(event) {                 
           var title = event.target.getAttribute("title") 
           var srcAttr = event.target.getAttribute("src")       
           resetOtherImages(title)    

           switch (title) { 

              case "info":
                 if (srcAttr == "images/information_selected.png") {                   
                     $(this).attr('src', 'images/information_unselected.png')                    
                 } else {                          
                     $(this).attr('src', 'images/information_selected.png')
                 }  
              break;  

              case "Up":
                 if (srcAttr == "images/up_selected.png") {                     
                     $(this).attr('src', 'images/up_unselected.png')                     
                 } else {                          
                     $(this).attr('src', 'images/up_selected.png')
                 }                                 
               break;

               // ...

alternatively you could factor further, either by using an array or the following switchSrc function:

  • the || will stop searching after the first match: link
  • a function indicates whether it matched by returning true (or false)

Example:

if title == "Up" then the function will call:

switchSrc(event, "info", 
                 'images/information_unselected.png',
                 'images/information_selected.png')

it evaluates to false (no match) so the function proceeds further by calling:

switchSrc(event, "Up",
                 'images/up_unselected.png',
                 'images/up_selected.png')

which evaluates to true (it's a match) so the function stops there. See the link provided.

$(".tt").click(function(event) {                 
           var title = event.target.getAttribute("title")        
           resetOtherImages(title) 

           var handled = 
              switchSrc(event, "info", 
                               'images/information_unselected.png',
                               'images/information_selected.png')
           || switchSrc(event, "Up",
                               'images/up_unselected.png',
                               'images/up_selected.png')
           || switchSrc(event, "Down",
                               'images/down_unselected.png',
                               'images/down_selected.png')
           || switchSrc(event, "SE",
                               'images/se_unselected.png',
                               'images/se_selected.png')
           || switchSrc(event, "Arc",
                               'images/arc_unselected.png',
                               'images/arc_selected.png')        
        })  

        function switchSrc(event, title, srcAttr1, srcAttr2) {
            var curTitle = event.target.getAttribute("title")
            var curSrcAttr = event.target.getAttribute("src")

            if (curTitle != title) {
               return false
            }

            if (curSrcAttr == srcAttr1) {                   
               $(this).attr('src', srcAttr2)                    
            } else {                          
               $(this).attr('src', srcAttr1)
            }
            return true         
        }

        function resetOtherImages(title) {
            $('.t').each(function(){
                if ($(this).attr('src') !== title) {
                  var newName = changeName($(this).attr('src'))
                  $(this).attr('src',newName)   
                }
            });
        }

        function changeName(imgName) {    
           var pos   = imgName.indexOf("_");
           var first = imgName.substring(0,pos);
           return first.concat("","_unselected.png");
        }
| improve this answer | |
\$\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.