Getting images and saving them to localstorage

Question

The code in this project accepts an image URL and creates an <img> tag dynamically, using the image URL as the src attribute value. The image *name *and URL are then saved to localStorage. The page can be viewed here.

I really want to improve this code. I can't say if it's a good approach. I can't really think of anything else on how to accomplish this but it works. What I really want to know is if I can make this simpler. I think I kind overdid it. Did I?

/*jshint esversion: 6 */


function saveImages(name, value) {
    if (typeof(Storage) !== undefined) {
        localStorage.setItem(name, value);
    }
    else {
        alert("LocalStorage is not supported on your browser");
    }
}

function loadStorage() {
    const container = $(".images-container");
    let elem;
    var keys = Object.keys(localStorage),
        i = 0,
        key;
    for (; key = keys[i]; i++) {
        elem = "<img src='' " + "id='" + key + "' />";
        $(container).append(elem);
        let thisImg = document.getElementById(key);
        let source = localStorage.getItem(key);
        $(thisImg).attr('src', source);
    }
}

$(function() {
    "use strict";
    loadStorage();
    const create_btn = $("#createImage");
    const save_btn = $("#saveImages");
    const inputUrl = $("#ImageUrl");
    const inputName = $("#imageName");
    const container = $(".images-container");


    var createThisImage = function(name, url) {
        if ((name === "" && url === "") || (url === "")) {
            alert(" - expected input");
        }
        else if (name === "" || document.getElementById(name)) { //if name exists or name is empty then create a random name
            name = guid();
            name = new Image(url, name);
            name.createImage();
        }
        else {
            name = new Image(url, name);
            name.createImage();
        }

    };


    $(create_btn).on('click', function() {
        var new_image = inputName.val();
        var source = inputUrl.val();
        createThisImage(new_image, source);
    });


    $('img').on('click', function() {
        const identity = $(this).attr('id');
        const isValidDeletion = confirm("Delete " + identity + "?");
        if (isValidDeletion) {
            localStorage.removeItem(identity);
            window.location.reload(true);
        }
    });



    function Image(url, name) {
        this.img_url = url;
        this.img_name = name;
    }

    Image.prototype.createImage = function() {
        let id = this.img_name;
        let elem = "<img src='' " + "id='" + id + "' />";
        $(container).append(elem);
        let thisImg = document.getElementById(id);
        thisImg.src = this.img_url;
        saveImages(this.img_name, this.img_url);
        window.location.reload(true);
    };


});








//Code from https://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript 
function guid() {
    return s4() + s4() + '-' + s4() + '-' + s4() + '-' +
        s4() + '-' + s4() + s4() + s4();
}

function s4() {
    return Math.floor((1 + Math.random()) * 0x10000)
        .toString(16)
        .substring(1);
}

Answer 1

loadStorage()

• Wierd selective use of jQuery.
  • container is already a jQuery object. Just do container.append(elem); no need to wrap it in jQuery again.
  • document.getElementById(key); could be shortened to $("#"+key)[0]
  • $(thisImg).attr('src', source); could just be thisImg.src = source

createThisImage()

• Why didn't you declare this function like the other ones?
• The conditional if ((name === "" && url === "") || (url === "")) is the exact same thing as if (url === "")

Image.prototype.createImage()

• Image is already a JS object, it represents an <img> object. Get rid of your Image constructor and use the native one.

  Image.prototype.createImage = function() {
      $(container).append(this);
      saveImages(this.id, this.src);
      window.location.reload(true);
  };
  

  This would then be used like this...

  img = new Image();
  img.id = name;
  img.src = url;
  img.createImage();
  

Answer 2

I suggest you read Stop Writing Slow Javascript. I know it starts off by bashing jQuery but beyond that it has some useful considerations involving DOM interactions. After reading that article, I suggest the following changes.

DOM lookups

The function loadStorage() performs a DOM look-up for elements with class images-container, which also happens in the DOM-loaded (jQuery) callback. Instead of doing the lookup twice, make the scope outside both functions (would need to use let instead of const).

let container;
function loadStorage() {
    ...
    //utilize container
}
$(function() { //jQuery DOM loaded callback
    container = $('.images-container');
    loadStorage();

Additionally, the function loadStorage() inserts images into the DOM, then immediately looks them up just to set the src attribute. Because jQuery is available, one can supply the attributes when creating the item using jQuery(html, attribute).

So the code to add each item could go from:

    elem = "<img src='' " + "id='" + key + "' />";
    $(container).append(elem);
    let thisImg = document.getElementById(key);
    let source = localStorage.getItem(key);
    $(thisImg).attr('src', source);

And be simplified to:

elem = $('<img>', {"id": key, "src": images[key]});
container.append(elem);

localStorage considerations

It might be wise to put the images in a sub-property of the localStorage, otherwise if other data needed to be stored there, the current code would attempt to load images based on that data. For example, if the following data needed to be stored:

localStorage.setItem("name", "jcstine");

Then the current code would attempt to load an image with the src attribute set to "jcstine".

An alternative is to store the images in a property - e.g. images within localStorage. Unfortunately objects need to be serialized before storing in localStorage - use JSON.stringify() and JSON.parse() to unserialize.

function saveImages(name, value) {
  if (typeof(Storage) !== undefined) {
    let images = JSON.parse(localStorage.getItem('images'));
    if (typeof(images) !== 'object') {
      images = {};
    }
    images[name] = value;
    localStorage.setItem('images', JSON.stringify(images));

Using a document fragment

When appending multiple items to the DOM, it can be best to use a DocumentFragment as a temporary container until all items are ready. That way there will be fewer "page reflows which will result in better performance"¹.

function loadStorage() {
  let elem, fragment = document.createDocumentFragment();    
  const images = JSON.parse(localStorage.getItem('images'));

  for (var key in images) {
    elem = $('<img>', {"id": key, "src": images[key]});
    $(fragment).append(elem);
  }
  container.append(fragment);
}

Image.prototype.createImage()

After adding the image to the DOM, this function calls saveImages() and then window.location.reload(). Because of that, it is basically superfluous to add the image to the DOM and then reload the page. That function could basically be reduced to the following, utilizing the loadStorage() function to add the images.

Image.prototype.createImage = function() {
    saveImages(this.img_name, this.img_url);
    window.location.reload(true);    
}

logic in createThisImage()

As I wrestled a bear once. mentioned, the first conditional can be reduced to simply url === "" or !url.

The second conditional (i.e. if (name === "" || document.getElementById(name)) {) is a little confusing. The comment is //if name exists or name is empty then create a random name but the second part looks up a DOM element. If that DOM element exists should it really still add the image? Perhaps it should skip adding the image?

Re-using variables can hurt readability. Perhaps instead of overwriting name with the Image instance, use a new variable.

And if the name is blank, then use short-circuiting OR to call guid().

var createThisImage = function(name, url) {
    if (!url) {
      alert(" - expected input");
    } else { 
      const newImage = new Image(url, name || guid());
      newImage.createImage();
    }
  };

See a demo of the updated code in this jsfiddle,

---

¹_{https://ilikekillnerds.com/2015/02/stop-writing-slow-javascript/}