patternjavascriptMinor
Selecting multiple images
Viewed 0 times
multipleimagesselecting
Problem
I wrote this code to allow users to select multiple images, and each image can be selected multiple times. It feels convoluted, though, and prone to errors, such as forgetting to do something associated with an action.
How can I make it more object-oriented or reusable?
```
var images = [];
function removeImage(image) {
while((index = images.indexOf(image)) !== -1) {
images.splice(index, 1);
}
}
function decrementImage(image) {
var index = images.indexOf(image);
images.splice(index, 1);
}
function countOccurrences(image) {
var occurrences = 0;
for (i = 0; i < images.length; i++) {
if (images[i] === image) occurrences += 1
}
return occurrences;
}
$('#image-container').on('click', 'img', function () {
var image = $(this),
source = image.attr('src');
image.data('selected', true).addClass('selected').siblings('.controls').show();
images.push(source);
image.siblings('span.count').html(countOccurrences(source));
});
$('#image-container').on('mouseleave', '.image-wrapper', function () {
$(this).children('.controls').hide();
});
$('#image-container').on('mouseenter', 'img', function () {
var image = $(this),
controls = image.siblings('.controls');
if (image.data('selected') === true) {
controls.show();
}
});
$('#image-container').on('click', '[data-action="remove"]', function () {
var controls = $(this).parents('div.controls'),
image = controls.siblings('img')
source = image.attr('src');
image.data('selected', false).removeClass('selected');
controls.hide();
removeImage(source);
image.siblings('span.count').html(countOccurrences(source));
});
$('#image-container').on('click', '[data-action="increment"]', function () {
var image = $(this).parents('.controls').siblings('img'),
source = image.attr('src');
images.push(source);
image.siblings('span.count').html(countOccurrences(source
How can I make it more object-oriented or reusable?
```
var images = [];
function removeImage(image) {
while((index = images.indexOf(image)) !== -1) {
images.splice(index, 1);
}
}
function decrementImage(image) {
var index = images.indexOf(image);
images.splice(index, 1);
}
function countOccurrences(image) {
var occurrences = 0;
for (i = 0; i < images.length; i++) {
if (images[i] === image) occurrences += 1
}
return occurrences;
}
$('#image-container').on('click', 'img', function () {
var image = $(this),
source = image.attr('src');
image.data('selected', true).addClass('selected').siblings('.controls').show();
images.push(source);
image.siblings('span.count').html(countOccurrences(source));
});
$('#image-container').on('mouseleave', '.image-wrapper', function () {
$(this).children('.controls').hide();
});
$('#image-container').on('mouseenter', 'img', function () {
var image = $(this),
controls = image.siblings('.controls');
if (image.data('selected') === true) {
controls.show();
}
});
$('#image-container').on('click', '[data-action="remove"]', function () {
var controls = $(this).parents('div.controls'),
image = controls.siblings('img')
source = image.attr('src');
image.data('selected', false).removeClass('selected');
controls.hide();
removeImage(source);
image.siblings('span.count').html(countOccurrences(source));
});
$('#image-container').on('click', '[data-action="increment"]', function () {
var image = $(this).parents('.controls').siblings('img'),
source = image.attr('src');
images.push(source);
image.siblings('span.count').html(countOccurrences(source
Solution
Your code is actually very good.
I'd advice against using OOP there. It's an overused tool which, when not necessary, only makes things more complex. JavaScript works very well without it. I'd recommend a more functional style. So, if you want to make your code more reusable, you could abstract your functions to work with any kind of collection. You could also use existing functions to define others:
Then, counting an image would be, for example:
But before I'd suggest checking some functional JavaScript libraries such as
I'd advice against using OOP there. It's an overused tool which, when not necessary, only makes things more complex. JavaScript works very well without it. I'd recommend a more functional style. So, if you want to make your code more reusable, you could abstract your functions to work with any kind of collection. You could also use existing functions to define others:
function removeFirst(array,obj) {
array.splice(array.indexOf(obj), 1);
};
function without(array,obj){
return array.filter(function(element){
return element !== obj;
});
};
function count(array,obj){
return array.length - without(array,obj).length;
};Then, counting an image would be, for example:
count(images,my_image);But before I'd suggest checking some functional JavaScript libraries such as
underscore.js, which already implement similar functions in very clever manners. Using them in your daily code will make it smaller, simpler, more readable and robust.Code Snippets
function removeFirst(array,obj) {
array.splice(array.indexOf(obj), 1);
};
function without(array,obj){
return array.filter(function(element){
return element !== obj;
});
};
function count(array,obj){
return array.length - without(array,obj).length;
};count(images,my_image);Context
StackExchange Code Review Q#23489, answer score: 2
Revisions (0)
No revisions yet.