Simplifiying/modularizing my code in javascript

Question

I know that I should follow the DRY principle in coding. However, I am not that into javascript so I want to ask how to make the code below more readable and maintanable.

$('#frontfile_v').change(function(){
        reader = Main.Mod.image_change(this);
        reader.onload = frontvImageIsLoaded;
    });
    $('#rearfile_v').change(function(){
        reader = Main.Mod.image_change(this);
        reader.onload = rearvImageIsLoaded;
    });
    $('#rightfile_v').change(function(){
        reader = Main.Mod.image_change(this);
        reader.onload = rightvImageIsLoaded;
    });
    $('#leftfile_v').change(function(){
        reader = Main.Mod.image_change(this);
        reader.onload = leftvImageIsLoaded;
    });
    //called after an image file has been chosen
    function frontvImageIsLoaded(e) {
        $("#frontimagepreview").attr('src', e.target.result);
        $("#frontpreview-msg").css('color', 'green');  
    };
    function rearvImageIsLoaded(e) {
        $("#rearimagepreview").attr('src', e.target.result);
        $("#rearpreview-msg").css('color', 'green');   
    };
    function rightvImageIsLoaded(e) {
        $("#rightimagepreview").attr('src', e.target.result);
        $("#rightpreview-msg").css('color', 'green');  
    };
    function leftvImageIsLoaded(e) {
        $("#leftimagepreview").attr('src', e.target.result);
        $("#leftpreview-msg").css('color', 'green');   
    };

This is the code for Main.Mod.image_change()

 var image_change = function handleFileImageChange(obj){
            //holds the image preview object

            var file = obj.files[0];
            var imagefile = file.type;
            var match= ["image/jpeg","image/png","image/jpg"];

            if(!((imagefile==match[0]) || (imagefile==match[1]) || (imagefile==match[2]))){
              alert("Incorrect image file. You still be able to upload this form but the system " + 
               "will be using the default image.");
              $("#preview-msg").css('color', 'red');
              return false;
            }else{
                var reader = new FileReader();
                //reader.onload = imageIsLoaded;
                reader.readAsDataURL(obj.files[0]); 

              return reader;
            }
        };

The code above, will handle file input change event then change img src base on the file input.

I know the code i wrote really sucks since I have to repeat my code several times. How can I implement it in a more efficient way?

Thanks.

seems this question more for codereview.stackexchange.com

Grundy
– Grundy

2015-05-17 12:50:30 +00:00
Commented May 17, 2015 at 12:50 — Grundy
– Grundy, Commented May 17, 2015 at 12:50

Grundy · Accepted Answer · 2015-05-17 12:46:56Z

1

use , to combine selectors:

$('#frontfile_v,#rearfile_v').change(function(){
    // ... 
})

The "change" event will be bound to every object matched by the selector. This way you don't need to duplicate the binding.

Merge the "image loaded" functions into one function by passing parameters:

var idsMap = {
   leftfile_v : {preview : '#frontimagepreview', msg : '#frontpreview-msg'},
   // etc...
};

$('#leftfile_v,#rearfile_v').change(function(){
    var ids = idsMap[$(this).attr('id')];
    reader = Main.Mod.image_change(this);
    reader.onload = function(e) {
        imageIsLoaded(e, ids.preview, ids.msg);
    };
});

function imageIsLoaded(e, preview, msg) {
    $(preview).attr('src', e.target.result);
    $(msg).css('color', 'green');  
};

edited May 17, 2015 at 12:46

Grundy

13.4k3 gold badges39 silver badges57 bronze badges

answered May 17, 2015 at 12:37

Malki

2,4058 gold badges35 silver badges64 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Grundy · Accepted Answer · 2015-05-17 13:04:11Z

Yet another variant. As say @Malki: use comma in selector

$('#frontfile_v, #rearfile_v,#rightfile_v,#leftfile_v').change(function(){
    var id = this.id.replace(/file_v$/,'');
    reader = Main.Mod.image_change(this);
    if(reader){ //for case when `image_change` return not "false"
        // use mode generic function
        reader.onload = function(e){
            $("#"+id+"imagepreview").attr('src', e.target.result);
            $("#"+id+"preview-msg").css('color', 'green'); 
        };
    }
});

As for handleFileImageChange you need use Array.indexOf function

var image_change = function handleFileImageChange(obj){
    //holds the image preview object

    var file = obj.files[0];
    var imagefile = file.type;
    var match= ["image/jpeg","image/png","image/jpg"];

    if(match.indexOf(imagefile) == -1){
        alert("Incorrect image file. You still be able to upload this form but the system will be using the default image.");
        $("#preview-msg").css('color', 'red');
        return false; 
    }else{
        var reader = new FileReader();
        //reader.onload = imageIsLoaded;
        reader.readAsDataURL(file); //you not need use "obj.files[0]" because you already save it in "var file"

        return reader;
    }
};

Collectives™ on Stack Overflow

Simplifiying/modularizing my code in javascript

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related