Javascript-форум (https://javascript.ru/forum/)
-   Общие вопросы Javascript (https://javascript.ru/forum/misc/)
-   -   Прошу помочь оптимизировать код (https://javascript.ru/forum/misc/49225-proshu-pomoch-optimizirovat-kod.html)

Master_Sergius 03.08.2014 19:36

Прошу помочь оптимизировать код
 
Написал черновой вариант помощника принятия решений. Но, поскольку с джаваскриптом Я ещё не сильно дружу, код получился немного грубый. Прошу помочь любыми подсказками, объективными критиками, советами и т.п. Код получился на 220 строк, простите, если слишком большой сюда вставляю.

function clear()
{
  /*
    Cheat way to clear document body
  */
  document.body.innerHTML = '';
}

helper = new Questions();


function Questions()
{
  /*
    Main object, which manages questions and processes answers
  */
  this.question_count = 5;

  this.process_answer = function (answer, num) {
  /*
    Stores answers and calculates approximate decision
    Be careful with nums, first question - first process, etc.
  */
    if (num == 1)
    {
      this.yes = 1;
      this.no = 1;
      if (answer == 'low')
        this.koef = 2
      else if (answer == 'medium')
        this.koef = 3
      else this.koef = 4;
    }
    if (num == 2)
    {
      if (answer == 'yes')
        this.yes *= this.koef
      else this.no *= this.koef;
    }
    if (num == 3)
    {
      if (answer == 'yes')
        this.no *= this.koef
      else this.yes *= this.koef;
    }
    if (num == 4)
    {
      if (answer == 'yes')
        this.yes *= this.koef
      else this.no *= this.koef;
    }
    if (num == 5)
    {
      if (answer == 'yes')
        this.yes *= this.koef
      else this.no *= this.koef;
    }
    if (num == this.question_count)
      this.show_result();
  }

  this.show_result = function () {
    /*
      Shows result in understandable form
    */
    clear();
    var yes = this.yes / (this.yes + this.no) * 100;
    var no = this.no / (this.yes + this.no) * 100;
    result = 'Result:<br><br>YES: ' + yes + '%, NO: ' + no + '%';
    var result_msg = document.createElement("p");
    result_msg.setAttribute('class', 'result_msg');
    result_msg.setAttribute('id', 'result_msg');
    result_msg.innerHTML = result;
    document.body.appendChild(result_msg);
    // very bad cheater stop
    throw new Error('just stop');
  }

  this.next = function (num=0) {
  /*
    Invokes by click on "next" button.
    Gets answer, moves to next question.
  */
    num += 1;
    if (num > 1)
    {
      answer = get_checked_value("answer");
      if (answer == '')
      {
        this.show_warning('You must choose one option!');
        num -= 1;
        return NaN;
      } else
      this.process_answer(answer, num-1);
    }
    clear();
    if (num == 1)
    {
      question_label = 'Importance of decision';
      answers = ['low', 'medium', 'high'];
      this.show_question(question_label, answers);
    }
    if (num == 2)
    {
      question_label = 'What decision you prefer at start?';
      answers = ['yes', 'no'];
      this.show_question(question_label, answers);
    }
    if (num == 3)
    {
      question_label = 'Has the answer "YES" any negative consequences?';
      answers = ['yes', 'no'];
      this.show_question(question_label, answers);
    }
    if (num == 4)
    {
      question_label = 'Has the answer "NO" any negative consequences?';
      answers = ['yes', 'no'];
      this.show_question(question_label, answers);
    }
    if (num == 5)
    {
      question_label = 'What decision do you prefer now?';
      answers = ['yes', 'no'];
      this.show_question(question_label, answers);
    }
    // create "next" button
    var next = document.createElement("input");
    next.setAttribute('type','button');
    next.setAttribute('value','next');
    next.onclick = function() { helper.next(num) }
    document.body.appendChild(next);
  } // end of next function

  this.show_warning = function(msg) {
    /*
      Shows warning message. Warning message can be only one at the same time
    */
    if (!document.getElementById('warning'))
    {
      var warning = document.createElement("p");
      warning.setAttribute('class', 'warning');
      warning.setAttribute('id', 'warning');
      warning.innerHTML = msg;
      document.body.appendChild(warning);
    }
  }

  this.show_question = function(question_label, answers) {
  /*
    Shows question with variants of answers on HTML page
  */
    var question = document.createElement("p");
    question.setAttribute('class', 'question');
    question.setAttribute('id', 'question');
    question.innerHTML = question_label;
    document.body.appendChild(question);
    legend = 'Choose on of:';
    count = answers.length;
    args = {legend:legend, count:count};
    for (i=1; i<=count; i++)
    {
      args['rb'+i] = {id:'rb'+i, value:answers[i-1], label:answers[i-1]}
    }
    create_radiogroup(args);
  } // end of show_question function
}


function create_radiogroup(args)
{
  /*
   Creates radiogroup, i.e. fieldset, which contains radiobuttons

   Parameters:
     args: an object, must have next keys:
             legend - label for parent fieldset;
             count - amount of radio buttons;
             rbN - dict with id and value, N = 1..count;

   Example:
     {legend:'test', count:2, rb1:{id:'rb1', value:true, label:'yes'},
                              rb2:{id:'rb2', value=false, label:'no'}}
  */
  var fieldset = document.createElement("fieldset");
  var legend = document.createElement("legend");
  legend.innerHTML = args['legend'];
  fieldset.appendChild(legend);
  for (i=1; i<=args['count']; i++)
  {
    var option = document.createElement("input");
    option.setAttribute('type', 'radio');
    option.setAttribute('name', 'answer');
    option.setAttribute('id', args['rb'+i]['id']);
    option.setAttribute('value', args['rb'+i]['value']);
    var label = document.createElement("label");
    label.setAttribute('for',args['rb'+i]['id']);
    label.innerHTML = args['rb'+i]['label'];
    fieldset.appendChild(option);
    fieldset.appendChild(label);
    var linebreak = document.createElement("br");
    fieldset.appendChild(linebreak);
  }
  document.body.appendChild(fieldset);
}

function get_checked_value(radiogroup_name)
{
  /*
    Gets the value of selected radiobutton from group
  */
  var selected = '';
  var group = document.getElementsByName(radiogroup_name);
  for (i=0; i<group.length; i++)
  {
    if (group[i].checked == true)
    {
      selected = group[i].value;
    }
  }
  return selected;
} // end of function get_checked_value

Aetae 03.08.2014 20:02

Цитата:

Сообщение от Rise (Сообщение 324103)
setAttribute заменить на свойство...

Зачем? Имхо setAttribute наглядней.

Sweet 03.08.2014 20:06

Master_Sergius, а если бы вопросов было не 5, а 105? Твой класс Questions занимал бы не 150, а 15000 строк кода?

Aetae 03.08.2014 20:10

Master_Sergius, для небольшого скрипта сойдёт. А так if'ы можно поменять на switch(а лучше объеденить одинаковые реакции в функции, а вопросы и ответы в отдельный массив, копипаст не рулит), именование функций с_нижним_подчёркиванием заменить на lowerCamelCase как принято в js. Также, для описания функций лучше сразу использовать jsdoc это поможет во множестве вещей, как то: работа ide, сжатие с помощью gcc, документация итд.

Цитата:

Сообщение от Rise (Сообщение 324106)
Чем наглядней не вижу преимуществ? Поясните.

Одинаковой длины, отделяет назначаемое значение скобочкам, ясно видно что делает. Само собой понятия наглядности и крсоты чисто субъективны, но никаких причин использовать именно свойства заместо атрибутов также нет.)

Master_Sergius 03.08.2014 23:07

Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )

Erolast 04.08.2014 05:40

Есть одно простое правило - если код пишется долго и нудно, скорее всего, в нем что-то не так. Хороший код пишется быстро и легко. То есть, повторю Aetae, одинаковый функционал должен быть вынесен в функции, функции должны иметь понятное назначение и данные должны быть отделены от логики.

Aetae 04.08.2014 09:00

Цитата:

Сообщение от Master_Sergius (Сообщение 324128)
Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )

Закон самообучения: видишь непонятное слово\аббривеатуру относящуюся к теме обучения - гуглишь и изучаешь. Вот и весь секрет. Иначе никуда не уедешь.
Учебник конкретно по js и на этом сайте отличный.

WorM32 04.08.2014 09:19

1. Я бы вынести все функции в прототип.
2. Пропущенные var перед объявлением переменной. Очень много где.
3. Метод show_result в конце кидает ошибку. Зачем?
4. Метод next. В одном из условий return NaN. Зачем?
5. Весь текст аля "'Importance of decision'" я бы вынес в константы куда нить повыше.
6. this.next = function (num=0) Это не работает. Правильно делать так.
/**
 * @param {number} [num=0]
 */
this. next = function (num) {
    num = num || 0;
    ...
}

7. Про атрибуты уже говорили. Я бы тоже использовал свойства, а не атрибуты. Запись короче и понятнее.

Erolast 04.08.2014 10:19

Цитата:

6. this.next = function (num=0) Это не работает. Правильно делать так.
Нет, это как раз правильно и это работает, но только в лисе. Значения параметров по умолчанию - часть еще неутвержденного ES6.

WorM32 04.08.2014 10:51

Цитата:

Сообщение от Erolast (Сообщение 324164)
Нет, это как раз правильно и это работает, но только в лисе. Значения параметров по умолчанию - часть еще неутвержденного ES6.

Это понятно, но вряд ли ТС писал данный код только под лису)


Часовой пояс GMT +3, время: 23:52.