5
\$\begingroup\$

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));
});

$('#image-container').on('click', '[data-action="decrement"]', function () {
    var image = $(this).parents('.controls').siblings('img'),
        source = image.attr('src');
    decrementImage(source);
    var count = countOccurrences(source)
    image.siblings('span.count').html(count);
    if (count < 1) {
        image.siblings('.controls').hide();
        image.data('selected', false).removeClass('selected');
    }
});

$(document).click(function () {
    console.log(images);
});

HTML

<div id="image-container">
    <div class="image-wrapper">
        <img src="http://i.imgur.com/nIZvrpS.jpg" width="100" height="100" data-selected="false" />
        <div class="controls">
            <a href="#" data-action="remove">Remove</a>
            <a href="#" data-action="increment">Increment</a>
            <a href="#" data-action="decrement">Decrement</a>
        </div>
        <span class="count">0</span>
    </div>
    <div class="image-wrapper">
        <img src="http://i.imgur.com/lUaIBXc.jpg" width="100" height="100" data-selected="false" />
        <div class="controls">
            <a href="#" data-action="remove">Remove</a>
            <a href="#" data-action="increment">Increment</a>
            <a href="#" data-action="decrement">Decrement</a>
        </div>
        <span class="count">0</span>
    </div>
    <div class="image-wrapper">
        <img src="http://i.imgur.com/DQzWfza.jpg?1" width="100" height="100" data-selected="false" />
        <div class="controls">
            <a href="#" data-action="remove">Remove</a>
            <a href="#" data-action="increment">Increment</a>
            <a href="#" data-action="decrement">Decrement</a>
        </div>
        <span class="count">0</span>
    </div>
</div>
\$\endgroup\$
2
\$\begingroup\$

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:

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.

\$\endgroup\$
2
\$\begingroup\$

Funny. I just answered a Facebook interview test question asking a similar thing. I'm out of time to cater it exactly to your question, but thought you might want to see it. Here was my answer:

<html>
<head>
    <script src="/assets/js/custom.js"></script>
</head>
<body>
    <div class="content">
        <img src="" class="image imageUser userCurrent" />
        <img src="" class="image imageGroup" />
    </div>
    <div class="siderbar">
        <img src="" class="image imageUser userGroup" />
    </div>
</body>

With this script:

$(function() {
function initImageControls() {
    var controlSettings = { "options": [ 
            {"Edit","edit"},
            {"View","view"},
            ...
            {"Delete","delete"}
            ] },
        controls = $( "<div>" )
            .addClass( "controls" )
            .addClass( "controls24" )
            .css("display", "none");
    $.each(controlSettings.options, function(value){
        $( "<div>" )
            .addClass( "control" )
            .addClass( "control-" + value[1] )
            .data( "value", value[1] )
            .appendTo( controls );
    });
    $( ".control" )
        .on("click", { "imageAction": $(this).data("value"), "target": $(this).parent().data("target") }, imageControl ); // passing action and object to perform it on

    $( "body" ).append(controls);

}
function imageControl(event) {
    var target = event.data.target,
        imageAction = event.data.imageAction;
    ...do whatever...
}
$( ".image" ) {
    .on({
          click: function(){
            showFullImage($(this));
          },
          mouseenter: function(){
            var $this = $(this),
                offset = $this. offset(),
                target = $this.data("id");
            $(".controls")
                .css( "right", offset.left + $this.width() )
                .css( "bottom", offset.top + $this.height() )
                .data("target",target)
                .show();
          }
          mouseleave: function(){
            $(".controls")
                .data("target",null)
                .hide();
          }
        });
}
});

and this CSS:

@import "config.scss";
@import "icons24x24/*.png";
/* ... */

/***** controls *****/
.controls { 
position: absolute;  }
.control { 
    @include ir; /* mixin to replace text with image - mixins included via config */
    float: left; }
.controls24 .control { /* separating this out allows micro and macro control sets */
    width: 24px;
    height: 24px;
    margin-left: 2px; }

.control-edit { @include icons24x24-sprite(icon-edit); // sprites generated via Compass
.control-edit:hover { @include icons24x24-sprite(icon-edit-on);
.control-edit { @include icons24x24-sprite(icon-view);
.control-edit:hover { @include icons24x24-sprite(icon-view-on);
/* ... */
.control-edit { @include icons24x24-sprite(icon-delete);
.control-edit:hover { @include icons24x24-sprite(icon-delete-on);

I think it's much better to remove all that excess code from the core HTML. I know this answer may create more questions than answers, but it's the direction you should take. Let me know if you have questions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.