HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Simple language quiz

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
quizsimplelanguage

Problem

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', {

Solution

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,


  ..
  ..


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

Code Snippets

<ul>
  <li class="choice">..</li>
  <li class="choice">..</li>
</ul>
function AudioFile(fileName) {
    var root = "../../Content/Audio/";
    this.mp3 = root + fileName + '.mp3';
    this.ogg = root + fileName + '.ogg';
}
function playAudio(audio) {
    var player = $('#jquery_jplayer');
    player.jPlayer('setMedia', {
        oga: audio.ogg,
        mp3: audio.mp3
    });
    player.jPlayer('play');
}
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);
    });
}

Context

StackExchange Code Review Q#5297, answer score: 5

Revisions (0)

No revisions yet.