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;
};
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Dec 12, 2016 at 16:16
\$\endgroup\$
5
  • \$\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\$ Commented Dec 12, 2016 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\$ Commented Dec 12, 2016 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\$ Commented Dec 12, 2016 at 16:53
  • \$\begingroup\$ This code seems incomplete. Where are correctAnswer and incorrectAnswer defined? \$\endgroup\$ Commented Dec 12, 2016 at 16:58
  • \$\begingroup\$ @MikeBrant there is the rest of the code \$\endgroup\$ Commented Dec 12, 2016 at 17:09

2 Answers 2

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.

answered Dec 12, 2016 at 21:03
\$\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

alecxe
17.5k8 gold badges52 silver badges93 bronze badges
answered Oct 9, 2017 at 8:23
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.