Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is there any way to simplify this code with loops or anything? I'm a beginner at JavaScript and my code is horrible. Please don't tell me to convert to jQuery or anything.

        function dragLeftdropLeft1(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topLeft1').style.display = "none";
            document.getElementById('topLeft2').style.display = "block";
        }
        function dragLeftdropLeft2(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topLeft2').style.display = "none";
            document.getElementById('topLeft3').style.display = "block";
        }
        function dragLeftdropLeft3(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topLeft3').style.display = "none";
            document.getElementById('topLeft4').style.display = "block";
        }
        function dragLeftdropLeft4(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topLeft4').style.display = "none";
            document.getElementById('topLeft5').style.display = "block";
        }
        function dragLeftdropLeft5(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topLeft5').style.display = "none";
            document.getElementById('topLeft1').style.display = "block";
        }
        function dragLeftdropRight1(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topRight1').style.display = "none";
            document.getElementById('topRight2').style.display = "block";
        }
        function dragLeftdropRight2(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topRight2').style.display = "none";
            document.getElementById('topRight3').style.display = "block";
        }
        function dragLeftdropRight3(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topRight3').style.display = "none";
            document.getElementById('topRight4').style.display = "block";
        }
        function dragLeftdropRight4(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topRight4').style.display = "none";
            document.getElementById('topRight5').style.display = "block";
        }
        function dragLeftdropRight5(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Left");
            document.getElementById('topRight5').style.display = "none";
            document.getElementById('topRight1').style.display = "block";
        }
        function dragRightdropLeft1(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topLeft1').style.display = "none";
            document.getElementById('topLeft2').style.display = "block";
        }
        function dragRightdropLeft2(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topLeft2').style.display = "none";
            document.getElementById('topLeft3').style.display = "block";
        }
        function dragRightdropLeft3(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topLeft3').style.display = "none";
            document.getElementById('topLeft4').style.display = "block";
        }
        function dragRightdropLeft4(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topLeft4').style.display = "none";
            document.getElementById('topLeft5').style.display = "block";
        }
        function dragRightdropLeft5(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topLeft5').style.display = "none";
            document.getElementById('topLeft1').style.display = "block";
        }
        function dragRightdropRight1(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topRight1').style.display = "none";
            document.getElementById('topRight2').style.display = "block";
        }
        function dragRightdropRight2(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topRight2').style.display = "none";
            document.getElementById('topRight3').style.display = "block";
        }
        function dragRightdropRight3(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topRight3').style.display = "none";
            document.getElementById('topRight4').style.display = "block";
        }
        function dragRightdropRight4(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topRight4').style.display = "none";
            document.getElementById('topRight5').style.display = "block";
        }
        function dragRightdropRight5(ev) {
            ev.preventDefault();
            var data=ev.dataTransfer.getData("Right");
            document.getElementById('topRight5').style.display = "none";
            document.getElementById('topRight1').style.display = "block";
        }

If you want to see the code in action, you can see the full HTML code at http://thomaswd.com/chopsticks. I am trying to make a chopsticks game using HTML and JavaScript.

share|improve this question

2 Answers 2

Abstract out the parts that are common to all of your functions. For instance:

function processEvent(evt, hideElem, showElem) {
    evt.preventDefault();
    document.getElementById(hideElem).style.display = "none";
    document.getElementById(showElem).style.display = "block";
    evt.dataTransfer.getData("Left");  //not sure why you were calling this, but I'll leave it in
}

Then you can implement your other functions like:

function dragLeftdropLeft1(ev) {
    processEvent(ev, 'topLeft1', 'topLeft2');
}

// and so on...

Note that if the nodes you are showing and hiding are adjacent in the DOM and one of them is the source of the event, you could probably rewrite the whole thing to work using a single function that takes advantage of event.target and element.nextSibling.

...or convert to jQuery :p.

share|improve this answer

It's easier if you set event handlers within javascript rather than inside the html code; this way it is simpler to pass variables to a single event handler. I don't understand why each of your event handlers has var data=ev.dataTransfer.getData but then doesn't do anything with the data; maybe this is something you will use later? And each of the top left and top right images is calling two separate functions when dropped (dragRightdropRight1(event),dragLeftdropRight1(event) etc), but as far as I can see the two functions do the same thing.

Anyway you can simplify as follows, with a single function drop that does something different depending on the name of the box (topLeft, topRight) and the image number (0-4).

var boxes = ['topLeft', 'topRight', 'bottomLeft', 'bottomRight'],
    images = {};

// Make a dictionary-like object containing all the image elements
for (var i = 0; i < boxes.length; i++) {
    images[boxes[i]] = document.getElementById('box' + (i + 1)).getElementsByTagName('img');
}

// Set event handlers for the 1st two boxes, topLeft and topRight
for (var i = 0; i < 5; i++) {
    images['topLeft'][i].ondrop = getDropFunction(ev, 'topLeft', i);
    images['topRight'][i].ondrop = getDropFunction(ev, 'topRight', i);
}

// Needed for reasons of variable scope
function getDropFunction(ev, boxName, imageNumber) {
    return function() {
        drop(ev, boxName, imageNumber);
    };
}

// Deal with the drop event, depending on which box and image was dropped
function drop(ev, boxName, imageNumber) {
    var nextImageNumber = (imageNumber + 1) % 5;
    ev.preventDefault();
    images[boxName][imageNumber].style.display = 'none';
    images[boxName][nextImageNumber].style.display = 'block';
    // as far as I can see this is all your code is doing
    // but you may want to add something that gets the data
    // from the ev and deals with it
}

If you use this kind of solution you can remove the ondrop attributes from the images in the HTML.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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