2
\$\begingroup\$

This is jQuery code which works like this: depending on a user choice it changes the color and background of some elements in the DOM.

The behavior is ok, is doing exactly what I want.

And as you may see, there is some code repeated. What's the best way to keep a good performance by doing a function in order to avoid repeating code?

function ArticleQuiz($el){
    this.$el = $el;
    this.answer = this.$el.data('answer');
    this.answerContainer = this.$el.find('.answer-container');
    this.result = this.$el.find('.result');
    this.correctAnswer = this.$el.find('.correct-answer');
    this.incorrectAnswer = this.$el.find('.incorrect-answer');
    this.quizLogic();
    return this;
}


ArticleQuiz.prototype.quizLogic = function(){
    var THIS = this;
    var answer;

    $('.masthead-article-quiz').parent().css('background', '#eee');
    $('.quiz-cols a', this.$el).click(function(e) {
        THIS.target = $(e.target);
        e.preventDefault();
        // exit if choice already made: users can't change their pick
        if ($(this).parents('.quiz-cols').parent().find('.white-font').length) return;
        // set class according to data-answer:
        if (THIS.target.hasClass('icon-false-shape')) {
            THIS.answerContainer.show();
            $(this).addClass('background-' + ((THIS.answer === false) ? 'green' : 'red')).addClass('white-font');
            THIS.result.addClass('font-' + ((THIS.answer === false) ? 'green' : 'red'));
            answer =  THIS.answer ? THIS.incorrectAnswer.show() : THIS.correctAnswer.show();
        }
        if (THIS.target.hasClass('icon-true-shape')) {
            THIS.answerContainer.show();
            $(this).addClass('background-' + ((THIS.answer === true) ? 'green' : 'red')).addClass('white-font');
            THIS.result.addClass('font-' + ((THIS.answer === true) ? 'green' : 'red'));
            answer =  !THIS.answer ? THIS.incorrectAnswer.show() : THIS.correctAnswer.show();
        }
    });
    return THIS;
};
\$\endgroup\$
  • \$\begingroup\$ Stackexchange supports snippets. It would be nice see the behavior working on a snippet if possible. if not try to do a js fiddle. \$\endgroup\$ – Bruno Costa Dec 12 '16 at 16:30
  • \$\begingroup\$ @BrunoCosta The behavior is ok, is doing exactly what I want. All I am asking is for is how can I improve my code. \$\endgroup\$ – Non Dec 12 '16 at 16:31
  • \$\begingroup\$ I believe you even though i didn't try to run the code on anything. But with javascript it's possible of you to show what you want to achieve in the user-interface, it's easier too see the desired behavior than parsing the code with eyes, or guessing what changes are made. \$\endgroup\$ – Bruno Costa Dec 12 '16 at 16:53
  • \$\begingroup\$ This code seems incomplete. Where are correctAnswer and incorrectAnswer defined? \$\endgroup\$ – Mike Brant Dec 12 '16 at 16:58
  • \$\begingroup\$ @MikeBrant there is the rest of the code \$\endgroup\$ – Non Dec 12 '16 at 17:09
1
\$\begingroup\$

The first thing that jumps out at me is the tight coupling it has with the current application in which it is being used. Why hard code all those CSS selectors? This class really cries out for configuration.

You might see a pattern like this used to achieve decoupling of class from config.

function ArticleQuiz($el, $config){

   this.config = {};
   ...
   init() {
       $.extend(this.config, this.defaultConfig, $config || {});
       ...
   }
   this.init();
}

ArticleQuiz.defaultConfig = {
    answerDataLabel = 'answer',
    answerContainerSelector = '.answer-container',
    ...
}

You should consider taking the approach of using meaningful class names to apply multiple CSS styles at once. So, rather than using specific semantic color and font class names, define a CSS class that encapsulates the entire state of being a correct/wrong answer. So taking this approach along with the suggestion to store configuration something might allow you to change this:

        $(this).addClass('background-' + ((THIS.answer === false) ? 'green' : 'red')).addClass('white-font');
        THIS.result.addClass('font-' + ((THIS.answer === false) ? 'green' : 'red'));
        answer =  THIS.answer ? THIS.incorrectAnswer.show() : THIS.correctAnswer.show();

Into something like:

       var classToApply = THIS.config.correctAnswerClass;
       if(THIS.answer === true) {
           classToApply = THIS.config.incorrectAnswerClass;         
       }
       $(this).addClass(classToApply).show();

Your QuizLogic function would not seem to have reason to be in the static (prototype) context, as you are using it as a constructor for your class. I would think this function should be within class function scope instead on prototype and should be call something like init() or similar common name for constructors in javascript. Note how I present the init() function in my first code example above.


Consider first populating your instance properties in the constructor rather than in the main class code. This gives you the ability to do things such as validate that the selectors do not return empty collections and similar activities needed to put the object in the proper state for use. If these validations fail, you could throw an error to prevent the object from being used in an improper state.

\$\endgroup\$
-3
\$\begingroup\$

You can use angular directives, for example:

<input type="button" value="set color" ng-click="myStyle={color:'red'}">
<input type="button" value="set background" ng-click="myStyle={'background-color':'blue'}">
<input type="button" value="clear" ng-click="myStyle={}">
<br/>
<span ng-style="myStyle">Sample Text</span>
<pre>myStyle={{myStyle}}</pre>

Example

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