patternjavascriptMinor
SVG mosaic creator
Viewed 0 times
svgmosaiccreator
Problem
Although my code works as expected, there are a few gotchas.
index.html
```
Mosaic
.container {
margin: 0 auto;
width: 50%;
}
.container ul {
list-style: none;
margin-left: 0;
}
var SVG_URL = 'http://localhost:8765/color/';
function httpGet(url) {
return new Promise(function(resolve, reject) {
var req = new XMLHttpRequest();
req.open('GET', url);
req.onload = function() {
if (req.status == 200) {
resolve(req.response);
} else {
reject(Error(req.statusText));
}
};
req.onerror = function() { reject(Error('Network Error')); };
req.send();
});
};
function getSvg(data) {
return httpGet(SVG_URL + data.hex)
.then(function(svg) {
return {svg: svg, x: data.x, y: data.y};
})
.catch(function(error) {
console.log(error);
});
};
function messageHandler(e) {
var chunks = e.data;
Promise.all(chunks.map(function(data) { return getSvg(data); }))
.then(function(response) {
self.postMessage(response);
// Close the woker to be garbage collected.
//self.close();
});
};
self.addEventListener('message', messageHandler, false);
(function(app) {
document.addEventListener('DOMContentLoaded', function
- A single row is not filled at once; instead, I can see partially filled rows during the rendering process(Fixed in the updated code below.).
- The application still feels slow; can it be optimized further?
- And the biggest thing, how can I make the code more OOPsy?
index.html
```
Mosaic
.container {
margin: 0 auto;
width: 50%;
}
.container ul {
list-style: none;
margin-left: 0;
}
var SVG_URL = 'http://localhost:8765/color/';
function httpGet(url) {
return new Promise(function(resolve, reject) {
var req = new XMLHttpRequest();
req.open('GET', url);
req.onload = function() {
if (req.status == 200) {
resolve(req.response);
} else {
reject(Error(req.statusText));
}
};
req.onerror = function() { reject(Error('Network Error')); };
req.send();
});
};
function getSvg(data) {
return httpGet(SVG_URL + data.hex)
.then(function(svg) {
return {svg: svg, x: data.x, y: data.y};
})
.catch(function(error) {
console.log(error);
});
};
function messageHandler(e) {
var chunks = e.data;
Promise.all(chunks.map(function(data) { return getSvg(data); }))
.then(function(response) {
self.postMessage(response);
// Close the woker to be garbage collected.
//self.close();
});
};
self.addEventListener('message', messageHandler, false);
(function(app) {
document.addEventListener('DOMContentLoaded', function
Solution
Obviously this code was posted nearly 3 years ago, so you have likely updated it, and since then ecmascript-6 has become a lot more prevalent/standard, so you could utilize many features/keywords of that specification to streamline this code. You could also use the fetch API or another promise-based XHR library to simplify the XHR promise code.
Your Questions
The application still feels slow; can it be optimized further?
You could consider using
And the biggest thing, how can I make the code more OOPsy?
You could use the ecmascript-6 class syntax, even though it is just "syntactical sugar over JavaScript's existing prototype-based inheritance"1.
Other feedback/suggestions
Let's look at the
Perhaps you know this already but this code could be simplified using
If you kept the
One micro-optimization might be to move the incrementing of i here:
To have the post-fix increment operator after the last usage of
1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes
Your Questions
The application still feels slow; can it be optimized further?
You could consider using
for...of or even regular for loops instead of functional iterators.And the biggest thing, how can I make the code more OOPsy?
You could use the ecmascript-6 class syntax, even though it is just "syntactical sugar over JavaScript's existing prototype-based inheritance"1.
Other feedback/suggestions
Let's look at the
forEach loop within renderRow() (defined inside drawMosiac()).var tiles = [];
rowData[i].forEach(function(data) {
var tile = new SVGTile(data.svg, data.x, data.y);
tiles.push(tile);
});Perhaps you know this already but this code could be simplified using
.map().const tiles = rowData[i].map(data => new SVGTile(data.svg, data.x, data.y));If you kept the
.forEach() then there isn't much point in assigning the value to tile right before it is pushed into the array - a linter should point that out as excess memory utilization. As was mentioned above, the functional code might be one reason why "the application still feels slow", so you might consider using a for loop if necessary.One micro-optimization might be to move the incrementing of i here:
res.push(new Tile(data.subarray(i * 4, i * 4 + 3), col, row));
i++;To have the post-fix increment operator after the last usage of
i within the subarray() call:res.push(new Tile(data.subarray(i * 4, i++ * 4 + 3), col, row));1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes
Code Snippets
var tiles = [];
rowData[i].forEach(function(data) {
var tile = new SVGTile(data.svg, data.x, data.y);
tiles.push(tile);
});const tiles = rowData[i].map(data => new SVGTile(data.svg, data.x, data.y));res.push(new Tile(data.subarray(i * 4, i * 4 + 3), col, row));
i++;res.push(new Tile(data.subarray(i * 4, i++ * 4 + 3), col, row));Context
StackExchange Code Review Q#134633, answer score: 2
Revisions (0)
No revisions yet.