5
\$\begingroup\$

This code is for a simple language quiz. It fetches two words and related audio files via a JSON call, presents the user with an image that matches one of the words, and challenges the user to make the match. My code works but it's a little repetitious and ugly. I'd like to see how the experts would propose rewriting this.

function getNewWords() {
        {
            var Category = "animals";
            var BaseURL = "http://localhost:61741/VocabGame/play?cat=";
            var URL = BaseURL + Category;


            $.getJSON(URL, {
                tagmode: "any",
                format: "json"
            }, function (data) {
                var choiceA = data.nouns[0].Pinyin;
                var choiceB = data.nouns[1].Pinyin;
                $('#ChoiceA').html(choiceA);
                $('#ChoiceB').html(choiceB);

                var root = "../../Content/Audio/";
                var mp31 = root + data.nouns[0].Audio1 + ".mp3";
                var ogg1 = root + data.nouns[0].Audio1 + ".ogg";
                var mp32 = root + data.nouns[1].Audio1 + ".mp3";
                var ogg2 = root + data.nouns[1].Audio1 + ".ogg";

                attachMouseEnter(mp31, ogg1, mp32, ogg2);

                var random = Math.random();
                if (random >= 0.5) {
                    correctAnswer = "ChoiceA";
                    Search(data.nouns[0].English);
                }
                else {
                    correctAnswer = "ChoiceB"
                    Search(data.nouns[1].English);
                }

            });
        }

    };

    function playAudio1(mp31, ogg1) {
        var mp3_src = mp31;
        var oga_src = ogg1;

        $('#jquery_jplayer_1').jPlayer('setMedia', {
            oga: oga_src,
            mp3: mp3_src
        });

        $('#jquery_jplayer_1').jPlayer("play");
    };

    function playAudio2(mp32, ogg2) {
        var mp3_src = mp32;
        var oga_src = ogg2;

        $('#jquery_jplayer_1').jPlayer('setMedia', {
            oga: oga_src,
            mp3: mp3_src
        });

        $('#jquery_jplayer_1').jPlayer("play");
    };
        function attachMouseEnter(mp31, ogg1, mp32, ogg2) {
            $('#ChoiceA').die('mouseenter.audio1event');
            $('#ChoiceA').live('mouseenter.audio1event', function () {
                playAudio1(mp31, ogg1);
            });

            $('#ChoiceB').die('mouseenter.audio2event');
            $('#ChoiceB').live('mouseenter.audio2event', function () {
                playAudio2(mp32, ogg2);
            });
    }
\$\endgroup\$
2
  • 5
    \$\begingroup\$ I can't spot any difference between playAudio1 and playAudio2, so that's a pretty damp area. \$\endgroup\$ – Captain Giraffe Oct 11 '11 at 2:58
  • 2
    \$\begingroup\$ I think this should be moved to Code Review SE... \$\endgroup\$ – Ian Macalinao Oct 11 '11 at 3:18
5
\$\begingroup\$

I think if you generalize the problem to having multiple choices instead of just 2, it would make things a lot simpler. It gives the added flexibility of adding more choices later on if needed, but most importantly, you can cut down explicit references to element A, or element B, or audio A, or audio B, throughout your codebase.

Doing so would mean that you tweak the HTML slightly, and instead of referring to DOM elements directly by their id such as choiceA, or choiceB, you rely on their position or index. The HTML of all choices may then look something like,

<ul>
  <li class="choice">..</li>
  <li class="choice">..</li>
</ul>

Keeping that in mind, here are the changes I would suggest. Create an object that represents an audio to avoid worrying about passing mp3/ogg variables all over.

function AudioFile(fileName) {
    var root = "../../Content/Audio/";
    this.mp3 = root + fileName + '.mp3';
    this.ogg = root + fileName + '.ogg';
}

Your playAudio1 and playAudio2 functions are identical. They can accept the AudioFile object, and be combined into one.

function playAudio(audio) {
    var player = $('#jquery_jplayer');
    player.jPlayer('setMedia', {
        oga: audio.ogg,
        mp3: audio.mp3
    });
    player.jPlayer('play');
}

By getting away from two choices, you can treat the number of choices as an array, and attach the required event handler for each item in the array alike. Also it seems like you are rebinding the live events after each AJAX call. That's just a waste of resources. You could explicitly bind after each AJAX call, or attach live handlers on document load or something. Here's the revamped getNewWords and attachMouseEnter methods.

function getNewWords() {
    var Category = "animals";
    var BaseURL = "http://localhost:61741/VocabGame/play?cat=";
    var URL = BaseURL + Category;

    $.getJSON(URL, didGetNewWords);

    var containers = $('.choice');

    function didGetNewWords(data) {
        data.nouns.forEach(function(noun, index) {
            var container = containers.get(index);
            container.html(noun.Pinyin);
            var audio = new AudioFile(noun.Audio1);
            attachMouseEnter(container, audio);
        });

        // Pick a random answer by index
        var answerIndex = Math.floor(Math.random() * data.nouns.length);
        correctAnswer = containers.get(answerIndex).id;
        Search(data.nouns[answerIndex].English);
    }
}

// Play this audio when this element is hovered over
function attachMouseEnter(element, audio) {
    $(element).bind('mouseenter', function() {
        playAudio(audio);
    });
}
\$\endgroup\$
1
  • \$\begingroup\$ Glad you found it useful. I like @pllee's suggestion about passing the params to the getNewWords function as that makes things much more customizable. \$\endgroup\$ – Anurag Oct 11 '11 at 5:00
2
\$\begingroup\$

Wouldn't you just remove playAudio2 and rename playAudio1 to playAudio, then in attachMouseEnter, just call playAudio both times.

function attachMouseEnter(mp31, ogg1, mp32, ogg2) {
$('#ChoiceA').die('mouseenter.audio1event');
$('#ChoiceA').live('mouseenter.audio1event', function () {
    playAudio(mp31, ogg1);
});

$('#ChoiceB').die('mouseenter.audio2event');
$('#ChoiceB').live('mouseenter.audio2event', function () {
    playAudio(mp32, ogg2);
});

Also I would make a function that took an id to attach to, the event name and then mp3, ogg params and then attachMouseEnter would just call this twice passing in the dif mp31, 2 etc

\$\endgroup\$
1
\$\begingroup\$

As mentioned the playAudo functions do the same thing. I don't know any JQuery so correct me if I am wrong but I think something like this is a little more flexible :

    function getNewWords(categoryName, categoryValue) {

    var baseURL = "http://localhost:61741/VocabGame/play", data = {};
    data[categoryName] = categoryValue;

    $.post(baseUrl, data,
        function(data, textStatus, jqXHR){
          //process json
     }, 
    "json");
 }

 getNewWords('animals', 'cat')

Let JQuery handle all of the parameter logic and prepare for taking in all kinds of data (dogs, cows .etc :)

\$\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.