Manager for image and JSON assets of a game

Question

I would like to know what you think of this assets manager for a game. If it can be optimized or simplified? It allows you to download sprites, tiles, json files, and other images.

const images = {};
images.tiles = {};

exports.images = images;
/**
* Download an image and store it in the cache
* @param {string} url - Address of the image
* @param {string} name - Name of the image
* @param {function} onImageLoaded -Execute this function when all images are loaded
*/

const loadImage = (url, name, onImageLoaded) => {

    if (images[name]) return onImageLoaded(images[name]);

    const image = new Image();

    image.onload = () => {
        if (image.complete) {
            images[name] = image;
            return onImageLoaded(image);
        }
    };

    image.src = url;
};

/*
* Download all images 
* @param {object} images - 
* @param {function} onImagesLoaded -
*/

const loadImages = (images, onImagesLoaded) => {
    var imagesLoaded = 0;


    const onImageLoaded = () => {
        imageLoaded++;

        if (images.length != imagesLoaded) return ;
        onImagesLoaded();
    }

    for (const image of images)
        loadImage(image.url, image.name, onImageLoaded)
};


/*
* Download all tiles
* @param {object} tiles - Tiles to download 
* @param {function} onTilesLoaded - Function that is executed when the tiles are downloaded
*/

const downloadTiles = (tiles, onTilesLoaded) => {

    const images = tiles.map(tile => {
        const image = {};

        image.name = 'tile_' + tile.id;
        image.url =  'assets/tiles/' + image.name + '.png';

        return image;
    });

    loadImages(images, onTilesLoaded);
};

exports.downloadTiles = downloadTiles;

/*
* Download all the images and store them in cache
* @param {object} sprites - Sprites to download
* @param {function} onSpritesLoaded - Function that is executed when the sprites are downloaded
*/

const downloadSprites = (sprites, onSpritesLoaded) => {
    const images = sprites.map(sprite => {
        const image = {};

        image.name = 'sprite_' + sprite.id;
        image.url = 'assets/sprites/' + image.name + '.png';

        return image;
    })

    loadImages(images, onSpritesLoaded);
}





const toJSON = response => response.json();

/*
* @param {string} url - 
* @param {string} name -
*/

const loadJSON = (url, name) => 
    fetch(url, { method: 'GET' })
        .then(toJSON);

exports.loadJSON = loadJSON;

exports.loadImage = loadImage;
exports.loadImages = loadImages;


const getTileCoordinates = (image, index, width, height) => {
    var row = 0;
    var x = width * index, y;

    while(true){
        if (x < image.width) break ;
        x-=image.width;
        row++;
    };
    y = height * row;

    return {x, y};
};
exports.getTileCoordinates = getTileCoordinates;

Answer 1

Naming and Commenting

An important part of being a programmer is good naming.

Looking at your code at first glance it seemed fine, until I started to rewrite it (I always rewrite when reviewing). For good code I should be able to rewrite the code without the need to go back to the original source for clarification. In this example I was constantly going back to try and understand what you were doing.

The main reason was that the name of variables that did not match what the variables contained and conflicted with important global names.

Comments

To make it worse the comments confused things even further

So let's examine the comments

/**
* Download an image and store it in the cache
* @param {string} url - Address of the image
* @param {string} name - Name of the image
* @param {function} onImageLoaded -Execute this function when all images are loaded

Last line is completely wrong, maybe?

  /* @param {function} onImageLoaded - callback for image onload event

3rd line not helpful , maybe?

 /* @param {string} name - Name used to store image in images object

2nd line, address? its a url or more apt a relative path to the image resource

1st line store it in a cache? Can't find any cache, but looking at the code I found the images object which you could call a cache, and download no you are not downloading in the classical sense of the word.

Maybe the following more clearly describes what the function does.

  /* Creates and loads Image adding it as a named object to images

The the second function's comments are as clear as mud

/* Download all images 
 * @param {object} images - 
 * @param {function} onImagesLoaded -

maybe

 /* Create and load images from an array image descriptions
  * @param {object} images - array of image descriptions contains url and name
  * @param {function} onImagesLoaded - Callback function called when all 
  *                                    images in array have loaded.

I have to admit that the very first thing I do when reviewing code is remove all comments (they rarely help), so your comments did not confuse me, only when writing this did I read them.

If you are going to add comments they must make sense, for if they dont they only create confusion, and that will result in problems when you return in months to touch up the code.

Write comments addressed to a 3rd person that is not in your head space. Sure the images object is a cache, you know that but that is not obvious to anyone else.

Names

Names are short.

When you name a variable it is at the point where you define its scope, that scope gives the variable a context. You name a variable in its context and don't add redundant information that is clearly implied from the context.

For example you have the argument onImageLoaded for the function loadImage I would say that the 'Image' part of the name is rather obvious so why add it. onLoaded is shorter, does not create ambiguity.

Names create entities

When you name an object you create an entity in the mind of the person reading your code (that person will be you months from now). When you see that name you associate the entity. Associating a global name with multiple entities leads to confusion.

Global names should be unique across its scope. You used the name images defined as const images = {}; but in parts of the code you also use the name images to represent an array. So you will have to be checking is this the images in global scope, or the images as an array.

Also image is used to represent an Image in some places and an image description in other places, confusing...

General comments

• Use strict mode it will result in better code and runs faster which is good for games. Add "use strict"; to the top of the JS file.

• Don't forget semicolons

• Don't create blockless blocks

//bad
for (const image of images)
    loadImage(image.url, image.name, onImageLoaded)

// good
for (const image of images) {                         // define block start
     loadImage(image.url, image.name, onImageLoaded); // semicolon
}                                                     // define block  end

• You are not using standard export so that means the whole set of functions could be in global scope. Use a singleton or use the native export. Try to limit your exports, exporting every function is just a pain an will lead to problems.

• The function getTileCoordinates is out of place and does not belong here. BTW there is a better way to find the row

function getTileCoordinates(image, index, width, height){
    const tilesAcross = image.width / width | 0; // | 0 floors result
    return {
        x : width * (index % tilesAcross),
        y : (index / tilesAcross | 0) * height
    };
}

• Don't repeat. The functions downloadSprites and downloadTiles are repeats

• I could not work out why you are returning the result of the load callbacks. There is no consistency as you do it sometimes and other times not. Also you are passing the image as an argument to the callbacks but there is no indication that you use it. For the onload event you can access the image via this if needed. Note you will have to use a standard function declaration for the callback.

• There is no error checking. What do you do if the connection or a request is lost?

A rewrite

This is how I would have written your code. It is just a suggestion example and far from ideal. I do not know what you wanted exposed but I only exposed what was not called internally (assuming this is a module)

"use strict";
const images = {
    tiles : {},
};

function loadImage(imageDesc, onLoad) {
    var image = images[imageDesc.name];
    if (image) { onLoad() }
    else{
        image = images[imageDesc.name] = new Image();
        image.src = imageDesc.url;
        image.onload = onLoad;
    }
},

function loadImages(imageArray, onAllLoaded) {
    var imagesLoaded = 0;
    const onLoad = () => {
        if (imageArray.length === ++ imagesLoaded) { onAllLoaded() }
    }
    for (const imageDesc of imageArray) { loadImage(imageDesc, onLoad) }
},

function loadList(type, list, onLoad) {
    loadImages( 
        list.map(imageDesc => {
            const name = `${type}_${imageDesc.id}`;
            return  { name, url : `assets/${type}/${name}.png` };
        }),
        onLoad
    );
},

const assests = {
    images,
    loadTiles(tiles, onLoaded) { 
        loadList("tiles", tiles, onLoaded);
    },
    loadSprites(sprites, onLoaded) { 
        loadList("sprites", sprites, onLoaded);
    },
    loadJSON : (url, name) => fetch(url, { method: 'GET' }).then(response => response.json()),
};


export default assests;