3
\$\begingroup\$

I wrote a little code sample which:

  • load images
  • manipulate DOM (replace images)
  • store/retrieve related data using localStorage

$(document).on('ready', function(){

  $('#chooseFile').on('click', function (e) {
    url = $('#image-url').val();

    // check if it's empty
        if (url === "")
            alert('Please type a URL');     

        url = addHTTP(url);

    if (checkURL(url)){

      // get image size (and save dimensions in local storage)
      getImageDimensions(url, updateBackgroundDimensions);

      localStorage.setItem('myImageURL', url);
      updateBackgroundImage(url);

      $('#fileModal').modal('hide');
    }else{
      alert('Not a valid image!');
    }
  });

  $('#resetImage').on('click', function (e) {
    window.localStorage.clear();
    location.reload();
  });

  checkLocalStorage();
});

function checkLocalStorage(){
  if(localStorage.getItem('myImageURL') &&
    localStorage.getItem('myImageURLWidth') &&
    localStorage.getItem('myImageURLHeight')) {

    updateBackgroundDimensions(localStorage.getItem('myImageURLWidth'),localStorage.getItem('myImageURLHeight'));
    updateBackgroundImage(localStorage.getItem('myImageURL'));
  }
}

function updateBackgroundDimensions(w, h){
  $('#image').css('width', w);
  $('#image').css('height', h);
  $('#image').css('background-size', w + 'px ' + h + 'px');
}

function updateBackgroundImage(url){
  $('#image').css('background-image', 'url('+url+')');
}

function getImageDimensions(url, callback){
  var img = $('<img src="'+url+'"/>').load(function(){
    localStorage.setItem('myImageURLWidth', this.width);
    localStorage.setItem('myImageURLHeight', this.height);
    callback(this.width, this.height);
  });
}

function addHTTP(url) {
   if (!/^(f|ht)tps?:\/\//i.test(url)) {
      url = "http://" + url;
   }
   return url;
}

function checkURL(url) {
    return(url.match(/\.(jpeg|jpg|gif|png)$/) !== null);
}

Full demo here

It works, but many parts could be improved. To begin with:

  1. I believe these lines should be together, maybe wrapped into a function. Probably it would be better to have only one item on localStorage (an object with three properties):

    localStorage.setItem('myImageURLWidth', this.width);
    localStorage.setItem('myImageURLHeight', this.height);
    

    and

    localStorage.setItem('myImageURL', url);
    

    ...but they aren't, because in order to know the image's width and height, the image must be loaded beforehand. And it wouldn't make sense to pass the image's URL along for the callback function updateBackgroundDimensions() to use it.

  2. For the same reason,

    updateBackgroundImage(url);
    

    and

    updateBackgroundDimensions();
    

    also aren't together.

  3. Are there recommended practices for separating localStorage related code (ex: checkURL() and addHTTP()) into a module or something?

  4. This seems to be very inappropriate: getImageDimensions(url, updateBackgroundDimensions); What if I need to use getImageDimensions() with more than one image?

  5. How can I approach this particular problem by separating all the DOM-related code from the application logic?

These are the main points that bother me now. If there's anything else not mentioned that could be improved, I'd love to hear.

Obs: I'd preferably handle these situations without an external library, using only JavaScript, but if there are some helpful ones, they're also welcome.

Here I quickly rewrote that to make it more readable and to arrange a couple things together. There's a lot that could be improved yet (by separating DOM manipulation from localStorage and other logic for example). It may look like that there's no really need considering that it would be a very simple app like this, but that's just an example, I actually want to grasp all information I can that relates to code design and that can improve maintenance and scalability, because I still got some bad habits of writing script on a single document, and while I've been trying to avoid that, often it doesn't really make a big difference.

And even though I'm able to grasp some concepts like closures and other stuff, It's not that easy to apply all that to my daily coding, it's hard sometimes to let go the 'automatic mode' despite my effort.

\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

Interesting question,

  1. You should indeed put some things together

    1. But I would keep 3 distinct properties for a project of this small size
    2. updateBackgroundDimensions() could know the URL from the localStorage..
    3. localStorage.setItem('myImageURL', url); should only be called from 1 place, right before the dimensions are set.
  2. Here again, if you get all the information first, then you can call updateBackgroundImage(url); and updateBackgroundImage(url); in one go.

  3. getImageDimensions( having support for 1 image at a time is fine, if you have multiple images, call it in a loop ;)

Other than that, from a once over:

  • You do not need to capture img in getImageDimensions, the following works:

    function getImageDimensions(url, callback){
      $('<img src="'+url+'"/>').load(function(){
        localStorage.setItem('myImageURLWidth', this.width);
        localStorage.setItem('myImageURLHeight', this.height);
        callback(this.width, this.height);
      });
    }
    
  • You do need to write out e if you are not going to use it, the following works:

      $('#resetImage').on('click', function (e) {
        window.localStorage.clear();
        location.reload();
      });
    
  • You are not declaring url with var

Will attempt my version soonish.

\$\endgroup\$
2
\$\begingroup\$

A few tips that have helped me;

  • Try and abstract parts of your system into little modules. For example in this project we can make an image loader module. Give the module the methods to manipulate something, and then you can use a new instance of it in your code. This saves you from having to write the same thing later on because you can just reuse your existing code.
  • Abstract methods so that they do one thing only. It will usually make sense for you to write a little module with small methods that do one thing well, and then have your 'client' application handle the finer details. For example you have the get image dimensions function, perhaps later on another use case will only use the image loader for this one function. So keeping methods small/focused is beneficial.

Note that you could make a URL helper function if you wanted (for checkURL etc), however for the sake of brevity I've left those methods as is. Something to think about? :)

// Left as is
function addHTTP(url) {
   if (!/^(f|ht)tps?:\/\//i.test(url)) {
      url = "http://" + url;
   }
   return url;
}

function checkURL(url) {
    return(url.match(/\.(jpeg|jpg|gif|png)$/) !== null);
}

// Generic image loader that takes a jQuery selector and a local storage key
var ImageLoader = function(options) {
    this.$domElement = options.$domElement;
    this.identifier  = options.identifier; 
};

ImageLoader.prototype.getImageDimensions = function(url, callback) {
    $('<img src="' + url + '">').load(function() {
        callback(this.width, this.height);
    });
};

ImageLoader.prototype.updateElement = function(options) {
    this.$domElement.css({
        'width': options.width,
        'height': options.height,
        'background-size': options.width + 'px ' + options.height + 'px',
        'background-image': 'url(' + options.url + ')'
    });
};

ImageLoader.prototype.retrieveImage = function() {
    var image = JSON.parse(localStorage.getItem(this.identifier));
    image && this.updateElement(image);
};

ImageLoader.prototype.storeImage = function(options) {
    localStorage.setItem(this.identifier, JSON.stringify(options));
};

$(document).ready(function() {
    var loader = new ImageLoader({
        $domElement: $('#image'),
        identifier:  'myImage'
    });

    $('#chooseFile').on('click', function() {
        var url = $('#image-url').val().length && addHTTP($('#image-url').val());

        if (url && checkURL(url)) {
            loader.getImageDimensions(url, function(width, height) {
                var options = {
                    width: width,
                    height: height,
                    url: url
                };

                loader.storeImage(options);
                loader.updateElement(options);

                $('#fileModal').modal('hide');
            });
        } else {
            console.log('Not a valid image');
        }
    });

    $('#resetImage').on('click', function() {
        window.localStorage.clear();
        location.reload();
    });

    loader.retrieveImage();
});

So you can see here that in our application code we have delegated the DOM manipulation of the element itself to our image loader module. So the only things really left to do are to bind the events, validate URL input, and store the image(s) from the client. We also made it so that we can create multiple image loaders if we like and let them use different local storage bindings.

As a side note, I'm rather fond of JavaScript expressions, which is why you can see them here for the url validation. I recommend that you look into them as they can help write nice one liners. :)

I hope this helps!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.