iamjc015
iamjc015

Reputation: 2245

Simplifiying/modularizing my code in javascript

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.

Upvotes: 2

Views: 60

Answers (2)

Grundy
Grundy

Reputation: 13380

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;
    }
};

Upvotes: 1

Malki
Malki

Reputation: 2385

  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.

  1. 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');  
    };
    

Upvotes: 1

Related Questions