HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Dynamically load assets (CSS+images+controllers) on route resolve

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
routedynamicallycsscontrollersresolveassetsloadimages

Problem

I wrote a little factory to dynamically load assets in angular resolve. Is it well structured and can it be made prettier?

```
app.factory('loadAssets', function ($q) {

var jsPath = "scripts/",
cssPath = "css/",
imagesPath = "images/",
head = document.getElementsByTagName("head")[0]; // define starting paths for each file tile

return {
startLoad: function (url) {

var fileType = url.split(".");
fileType = fileType[fileType.length - 1]; // we must get (length - 1) because filenames like jquery.min.js

/* prevent duplicate loading -
check if url exist in already loaded assets

1. If asset exist, return true and quit.
2. If asset doesn't exist - add it.
3. If loadedAssets is undefined (first time you load something) - create this array.
*/

if (this.loadedAssets == undefined || this.loadedAssets == null) {
this.loadedAssets = [];
}

if (this.loadedAssets.indexOf(url) >= 0) { // note that indexOf supported only in ie9+ , add fallback if you want to support legacy browsers.
return true;
} else {

this.loadedAssets.push.apply(this.loadedAssets, [url]);

// load js files
if (fileType == "js") {
var jsFile = document.createElement("script");
jsFile.src = jsPath + url;
head.appendChild(jsFile);
var waitforload = $q.defer();

jsFile.onload = function () {
waitforload.resolve(jsFile);
};
jsFile.onerror = function (e) {
waitforload.reject(e);
console.log("Could not load " + jsFile.src

Solution

From a once over:

-
This

var fileType = url.split(".");
fileType = fileType[fileType.length - 1]; // we must get (length - 1) because filenames like jquery.min.js


could be

//Get the last . separated string
var fileType = url.split(".").slice(-1);


.slice(-1) retrieves the last element in an array

-
This

if (this.loadedAssets == undefined || this.loadedAssets == null) {
  this.loadedAssets = [];
}


could be

this.loadedAssets == this.loadedAssets || [];


this will assign [] if this.loadedAssets evaluates to false (by being null or undefined)

-
This

if (this.loadedAssets.indexOf(url) >= 0) { // note that indexOf supported only in ie9+ , add fallback if you want to support legacy browsers.
  return true;
} else {


should be

// indexOf suppor only in ie9+ , add fallback if you want to support legacy browsers.
if (this.loadedAssets.indexOf(url) >= 0) { 
  return true;
}


The comment is on top which reads easier, but more importantly there is no else. The else was superfluous since you exit the function anyway if the url was already loaded

-
I would encode all the image extensions from if (fileType == "jpg" || fileType == "jpeg" || fileType == "png" || fileType == "gif") { into an array and use indexOf.

All in all, I like the code.
`

Code Snippets

var fileType = url.split(".");
fileType = fileType[fileType.length - 1]; // we must get (length - 1) because filenames like jquery.min.js
//Get the last . separated string
var fileType = url.split(".").slice(-1);
if (this.loadedAssets == undefined || this.loadedAssets == null) {
  this.loadedAssets = [];
}
this.loadedAssets == this.loadedAssets || [];
if (this.loadedAssets.indexOf(url) >= 0) { // note that indexOf supported only in ie9+ , add fallback if you want to support legacy browsers.
  return true;
} else {

Context

StackExchange Code Review Q#68251, answer score: 2

Revisions (0)

No revisions yet.