0
\$\begingroup\$

There are many related questions and answers but mostly with JQuery and/or from many years ago.

I prepared a version myself, and my question is whether this is currently (2020, hence ECMAScript 6 or similar) an optimal way to preload images in pure JavaScript. It appears to work (note the tests), although not all edge-cases are tested.

let preload_imgs = function(src_list, callback) {
    let loaded = 0;
    src_list.forEach((src) => {
        img = new Image();
        img.onload = () => {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = (err) => {
            console.log(err);
        };
        img.src = src;
    });
};

Here is an example usage:

let preload_imgs = function(src_list, callback) {
  let loaded = 0;
  src_list.forEach((src) => {
    img = new Image();
    img.onload = () => {
      if (++loaded == src_list.length && callback) {
        callback();
      }
    };
    img.onerror = (err) => {
      console.log(err);
    };
    img.src = src;
  });
};

let urls = ['https://i.picsum.photos/id/1000/5626/3635.jpg',
  'https://i.picsum.photos/id/10/2500/1667.jpg',
  'https://homepages.cae.wisc.edu/~ece533/images/cat.png',
  'https://homepages.cae.wisc.edu/~ece533/images/airplane.png'
];

let starttime = Date.now();
preload_imgs(urls, () => {
  console.log("All images preloaded in", Date.now() - starttime, 'ms.');
});

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to code review where we review working code and make suggestions on how to improve the code. We can't tell you if your code is working correctly, we need to assume you have tested it. \$\endgroup\$
    – pacmaninbw
    May 1, 2020 at 14:58
  • \$\begingroup\$ Please verify that your code does what you want it to and come back (also edit your question to reflect your findings), I'm certain I can help you with a cleaner solution :) \$\endgroup\$
    – Maanlamp
    May 1, 2020 at 15:23
  • \$\begingroup\$ Thanks for the feedback. I of course tested it several times (see in the example snippet too), what I meant is just that maybe there could be some potential problem in a scenario that I haven't thought of. Anyway, I just removed this part because it's not essential. \$\endgroup\$
    – gaspar
    May 1, 2020 at 15:43
  • \$\begingroup\$ To the reviewers: this question doesn't need to be closed for being broken, that reason doesn't apply here. Thank you. \$\endgroup\$
    – Mast
    May 1, 2020 at 19:21

1 Answer 1

8
\$\begingroup\$

Firstly, kudo's to you for trying to use vanilla javascript. Great choice.




🤔 The Question

Let us start with your question:

[Is this code] an optimal way to preload images in pure JavaScript?

Sorry, but that is very subjective so I cannot answer that. I am assuming good will, and thus will rather answer the following question I inferred from your original question:

Am I making good use of common best practices?

Now that is a question I can answer, and the simple answer is no. Sorry :)

Ofcourse, I will not leave you an answer without explanation.




🔁 Consistency and Deliberateness

Firstly, the biggest improvement one can make in their life as a programmer would be consistency. Do not listen to the people who argue about single vs double quotes, or tabs vs spaces. As a software engineer myself, I can wholeheartedly say that all that matters is when you make a choice, you stick to it (at least within the project -- you are allowed to change your mind). Most people do not mind style (it often is enforced to a company standard anyway), but whenever you use any function or syntax, a reader would assume you did so deliberately. Why did you choose both var function and x => y?

I have to admit that javascript tricked us by allowing three ways of creating functions, but that does not mean you should use them all together. Let us look at the ways and their reasons:

function name (args) { /*block*/ }

This is the function declaration statement. This just means that you declare a variable called name and assign the function to it. This was made primarily to support an imperative sequence of steps. It is my personal opinion that it should have been named procedure, because that is what it is.

var name = function (args) { /* block */ }

This is actually what happens when you declare a function, so there is no difference between this and the first type of function -- except for the more letters so I see no reason to use this notation. The only difference that could be made is through the storage modifier (i.e. var in this case), which I will tell you about a bit later.

var name = args => { /*block*/ }

This is javascript's lambda syntax. It allows for a very interesting way to write programs, but only if you stop appending blocks to it:

var add = (x, y) => {
  return x + y;
};
// is actually the same as
var add = (x, y) => x + y;

The arrow function (x => y) is a function that exists of only an expression, with an implied return. But if you use a block, you just turned it into a procedure again which negates the usefulness of the lambda/arrow notation. It can save you a lot of keystrokes, and, if I can add my opinion, looks much more clean.

Taking this into consideration, your code could look like this:

function preload_imgs (src_list, callback) {
    let loaded = 0;
    src_list.forEach(function(src) {
        img = new Image();
        img.onload = function() {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = function(err) {
            console.log(err);
        };
        img.src = src;
    });
};



📦 Storage and Mutability

Remember the storage modifiers I just talked about? Javascript also has three ways of declaring variables. What a nightmare, right? Well, there is some use to the differences -- have a look:

var x = ...

In the beginning there was only var. It is short for variable, and thus should just declare a variable, right? Nope, javascript is weird. I could go on about why it is the weirdest feature ever, but here is the link about var if you are interested. The main takeaway is that this notation does a lot of stuff behind the scenes, making it a common source of bugs. My opinion: don't use it.

let x = ...

Everyone who has ever used var probably meant to use let. It does not do any fancy stuff, it creates a variable and assigns a value, except that it is block scoped. This variable is only ever accessible within the confines of the closest block, i.e. anything within { curly brackets }. You use only let, so I assume you know of this advantage but I wanted to clarify it nonetheless.

const x = ...

Again, a very autological name: const creates a constant. A constant can only be declared once, and never overwritten. This might sound like a useless trait, but it is actually really important. One of the most famous bugs in the world was caused by mutability (no. 4 on the list, probably more) and cost a whopping $60 million. I am not saying that using const will always save you millions of dollars, but it disallows any overwriting of the value, meaning:

  • you cannot accidentally overwrite your or anyone else's functions and variables.
  • you can guarantee that its value will never change, so you can refer to it anywhere safely -- even in a for within a for within a for.
  • AT&T would have lost $60m less.

Coming back to deliberateness: you declare functions using let, implying you are going to change them, but you are not. I suggest using const to more clearly signal your intentions:

const preload_imgs = function (src_list, callback) {
    let loaded = 0;
    src_list.forEach(function(src) {
        const img = new Image();
        img.onload = function() {
            if (++loaded == src_list.length && callback) {
                callback();
            }
        };
        img.onerror = function(err) {
            console.log(err);
        };
        img.src = src;
    });
};



🤝 I promise to help

Talking about common best practises, there is one thing many javascript developers will run into. That thing is the internet. A great many developers have already walked this path before you, so you bet the industry has some handy stuff up its sleeves to help you become a better programmer.

Javascript has a fool-proof way of handling asynchronous tasks, such as preloading an image. For exactly this reason, promises were implemented. A Promise is a wrapper around an asynchronous task, so we can work with it synchronously. Without going into a lot of detail, let me summarise the most important parts of a Promise:

  • A promise allows for attaching "callbacks" for when the wrapped task completes
const response = new Promise(request);
response.then(doStuff);
  • A promise allows for attaching "callbacks" for when the wrapped task fails
const response = new Promise(request);
response.catch(handleError);

Can you see a similarity? Creating an Image and attaching onload/onerror callbacks sounds a lot like creating a Promise! Applying this knowledge to your example gives us the following code:

// Your "preload" function goes as follows
(src) => {
    img = new Image();
    img.onload = () => {
        if (++loaded == src_list.length && callback) {
            callback();
        }
    };
    img.onerror = (err) => {
        console.log(err);
    };
    img.src = src;
}
// Let's update it
const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    // onload is called with an event, not the image so we
    // want to specifically resolve with img.
    resolve(img);
  }
  img.onerror = reject;
  // onerror is called with the error, so we can directly assign "reject" to it.
  img.src = src;
});

You might notice the lack of "free variables" (foreign variables that do not contribute to the function itself). We do not use loaded or src_list because those alter state / are mutable and are a perfect souce for the $60m bug (actually, this is probably the exact type of function that caused the race condition in their software).

We have solved the preload function in a very clean way:

const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    resolve(img);
  }
  img.onerror = reject;
  img.src = src;
});

preload("https://s3.amazonaws.com/pix.iemoji.com/images/emoji/apple/ios-12/256/thumbs-up.png");
  .then(img => console.log("Preloaded", img))
  .catch(err => console.error("Failed", err));

BAM! You now have a function that preloads an image, and allows for individual succes and error handling. All we have to do now, is to apply this function to a list of srcs.




➰ Going in loops

You might have thought there were too many ways to declare functions or variables, but wait till you hear in how many ways you can loop over things in javascript!

There is for (let i = 0...), for ... in, for ... of, while, do ... while, and we have not even touched on higher order array traversal functions. I am just going to say it: you never really need a for loop. You can solve any array problem with array functions.

Side note: the Array.forEach function you used is a weird function. It is just a slower version of a for loop with different notation, and I just said you never need a for loop. Either use for (const src of src_list) or use map or filter, but never forEach (this is a guideline, if it prohibits you from being productive then go ahead and do it).

For example, there is the Array.map function. This function takes a transformer, which goes over every element of the array, and transforms that element yielding a new array. That might sound weird so here is an example:

const numbers = [1, 2, 3];
const double = x => x * 2;
const doubledNumbers = numbers.map(double);
// doubledNumbers: [2, 4, 6]

Hmm, taking an array of values and applying a function to every value sounds a lot like what you are trying to do:

// Your old code
src_list.forEach((src) => {
  img = new Image();
  img.onload = () => {
    if (++loaded == src_list.length && callback) {
      callback();
    }
  };
  img.onerror = (err) => {
    console.log(err);
  };
  img.src = src;
});

// Using our "preload" function from before
const requests = src_list.map(preload);
// requests: [Promise<Image>, Promise<Image>, Promise<Image>, ...]

Whoaaaa, one line! But we have not fully refactored yet, because we now have a list of promises that can just resolve at any time and in no particular order. You seem to have understood this problem, but you solved it in a very unsafe way.

let loaded = 0;
//...
img.onload = () => {
    if (++loaded == src_list.length && callback) { /* ... */ }
    //...
}

You keep track of the amount of loaded images in the callback function that triggers when any image is loaded. Pretty clever, but I would bet my house that keeping track of such things in this way will cause bugs. If you google "wait for all promises javascript", you learn of a built in function that takes care of all of this for you! Promise.all takes a list of promises, and returns a promise, that rejects when any promise in that list rejects, or resolves when all promises in the list have resolved. That is exactly what you are trying to achieve:

//using the "requests" list we created a couple examples back
const done = Promise.all(requests);
done
  .then(images => console.log("Preloaded all", images))
  .catch(err => console.error("Failed", err));
// > Preloaded all [Image, Image, Image, ...]



⌛ Conclusion

Hopefully you now have a better understanding of javascript, and the lovely built-in functions that make these things almost as easy as jquery, if not more. Maybe you even learned a lot of things along the way, maybe you were sighing and moaning because you knew most of this stuff -- as long as you learned anything I consider it a success.

To summarise:

  • Use any kind of function notation but be consistent and deliberate.
  • Use const for constants and let for variables (I never even use let at all).
  • Use Promises to wrap asynchronous tasks to allow for really useful manipulation functions and callbacks.
  • Familiarise yourself with higher-order functions so you never have to use loops again.

And voila, the refactored code:

const preload = src => new Promise(function(resolve, reject) {
  const img = new Image();
  img.onload = function() {
    resolve(img);
  }
  img.onerror = reject;
  img.src = src;
});

const preloadAll = sources =>
  Promise.all(
    sources.map(
      preload));

const sources = [
  'https://i.picsum.photos/id/1000/5626/3635.jpg',
  'https://i.picsum.photos/id/10/2500/1667.jpg',
  'https://homepages.cae.wisc.edu/~ece533/images/cat.png',
  'https://homepages.cae.wisc.edu/~ece533/images/airplane.png'];

preloadAll(sources)
  .then(images => console.log('Preloaded all', images))
  .catch(err => console.error('Failed', err));

Preload to your hearts desire.

\$\endgroup\$
3

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.