3
\$\begingroup\$

I'm really concerned about clean programming and nice readable code.

My goal is to change string (text), when user clicks on various products on the webpage. I noticed that my code is repeating and looks very simple (not professional). The most appropriate option would be to refactor my code and make it look better.

var text = $('.inner-container h1').text();
var changeString = $('.inner-container h1');

$('.ottogi').click(function() {
    if (text == 'Ottogi') {
        $(changeString).text('Ottogi');
    }
});
 $('.sajo').click(function() {
     if (text == 'Ottogi') {
        $(changeString).text('Sajo Hapyo');
    }
 });
  $('.natura').click(function(){
     if (text == 'Ottogi') {
        $(changeString).text('Natura Bogata');
    }
 });
$('.maloo').click(function(){
     if (text == 'Ottogi') {
        $(changeString).text('ТОО Малу');
    }
 });
$('.dongush').click(function(){
     if (text == 'Ottogi') {
        $(changeString).text('Dongsuh');
    }
 });
 $('.may').click(function(){
     if (text == 'Ottogi') {
        $(changeString).text('ООО Май');
    }
 });
\$\endgroup\$
  • \$\begingroup\$ I personally think that there is not enough info available. Ofcourse you can refactor your code, but as I see the code you currently have, I am unsure when it is executed. You seem to read the text once, and you use it in every eventhandler. Can you show how this is actually used? Can you not create a single clickhandler and then check which element was clicked and make your change? \$\endgroup\$ – Icepickle Aug 4 '17 at 8:32
  • \$\begingroup\$ @Icepickle you can refer to this website test1.testkz.ru \$\endgroup\$ – Nureke Duisekeyev Aug 4 '17 at 9:11
2
\$\begingroup\$

You could reduce duplication by using an object mapping the selectors (class names in this case) to their responses

const selectorToResponse = {
  '.ottogi': 'Ottogi',
  '.sajo': 'Sajo Hapyo',
  '.natura': 'Natura Bogata',
  '.maloo': 'TOO Maлy',
  '.dongush': 'Dongsuh',
  '.may': 'OOO Maй'
}

Note, that for the first case (.ottogi) you are changing the text to Ottogi if it is Ottogi which has no change, so it can be removed.

const selectorToResponse = {
  '.sajo': 'Sajo Hapyo',
  '.natura': 'Natura Bogata',
  '.maloo': 'TOO Maлy',
  '.dongush': 'Dongsuh',
  '.may': 'OOO Maй'
}

From here you could loop over the object entries using a for...in loop and define the event handlers. This way you only have to add further entries to selectorToResponse instead of duplicating.

const selectorToReponse = {
  '.ottogi': 'Ottogi',
  '.sajo': 'Sajo Hapyo',
  '.natura': 'Natura Bogata',
  '.maloo': 'TOO Maлy',
  '.dongush': 'Dongsuh',
  '.may': 'OOO Maй'
}

// Prefixing with $ to denote that is an element, not text etc
const $header = $('.inner-container h1')

for (const selector in selectorToResponse) {
  $(selector).click = function () {
    // using $header.text() inside function so that is current header text
    // `text` never changed in your example
    if ($header.text() === 'Ottogi') { // using === instead of ==
      const response = selectorToResponse[selector]

      $header.text(response)
    }
  }
}

Notes

Above uses strict equality comparison (===) instead of abstract equality comparison (==). https://developer.mozilla.org/en/docs/Web/JavaScript/Equality_comparisons_and_sameness

Solution uses const, an ES6 feature. The benefits are that selectors scope is confined to the for loop and that the variable identifier cannot be reassigned.

Depending on your desired browser compatibility it may or may not be available.

As @IcePickle pointed out, using var in the for...in loop would mean that selector is always .may in the click handlers due to var having different socping rules. See Creating closures in loops - A common mistake for a detailed explanation.

Using var and another closure:

var selectorToReponse = {
  '.ottogi': 'Ottogi',
  '.sajo': 'Sajo Hapyo',
  '.natura': 'Natura Bogata',
  '.maloo': 'TOO Maлy',
  '.dongush': 'Dongsuh',
  '.may': 'OOO Maй'
}

var $header = $('.inner-container h1')

for (var selector in selectorToResponse) {
  $(selector).click = (function (selector) {
    return function () {
      if ($header.text() === 'Ottogi') {
        var response = selectorToResponse[selector]

        $header.text(response)
      }
    }
  })(selector)
}
\$\endgroup\$
  • \$\begingroup\$ selector in your case will always be .may since the selector is part of the for - loop. You could be using the selector from the jQuery this context here. For the rest , I think it's in general a good approach. The only thing you would be unsure of is wether or not the OP had the intention to evaluate the header only at the start of his code, or throughout the changes. I cannot say what the intentions were as I don't know what the words mean... \$\endgroup\$ – Icepickle Aug 4 '17 at 9:27
  • \$\begingroup\$ @Icepickle, good spot - forgot about that quirk for a second due to using const/let \$\endgroup\$ – Craig Ayre Aug 4 '17 at 9:36
  • 1
    \$\begingroup\$ Good remark about let, didn't realize that it changes the behavior of a variable :) \$\endgroup\$ – Icepickle Aug 4 '17 at 10:18

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.