Javascript.RU

Создать новую тему Ответ
 
Опции темы Искать в теме
  #1 (permalink)  
Старый 03.08.2014, 19:36
Аспирант
Отправить личное сообщение для Master_Sergius Посмотреть профиль Найти все сообщения от Master_Sergius
 
Регистрация: 29.07.2014
Сообщений: 42

Прошу помочь оптимизировать код
Написал черновой вариант помощника принятия решений. Но, поскольку с джаваскриптом Я ещё не сильно дружу, код получился немного грубый. Прошу помочь любыми подсказками, объективными критиками, советами и т.п. Код получился на 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
Ответить с цитированием
  #2 (permalink)  
Старый 03.08.2014, 20:02
Аватар для Aetae
Тлен
Отправить личное сообщение для Aetae Посмотреть профиль Найти все сообщения от Aetae
 
Регистрация: 02.01.2010
Сообщений: 6,590

Сообщение от Rise Посмотреть сообщение
setAttribute заменить на свойство...
Зачем? Имхо setAttribute наглядней.
__________________
29375, 35
Ответить с цитированием
  #3 (permalink)  
Старый 03.08.2014, 20:06
Профессор
Отправить личное сообщение для Sweet Посмотреть профиль Найти все сообщения от Sweet
 
Регистрация: 16.03.2010
Сообщений: 1,618

Master_Sergius, а если бы вопросов было не 5, а 105? Твой класс Questions занимал бы не 150, а 15000 строк кода?
Ответить с цитированием
  #4 (permalink)  
Старый 03.08.2014, 20:10
Аватар для Aetae
Тлен
Отправить личное сообщение для Aetae Посмотреть профиль Найти все сообщения от Aetae
 
Регистрация: 02.01.2010
Сообщений: 6,590

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

Сообщение от Rise Посмотреть сообщение
Чем наглядней не вижу преимуществ? Поясните.
Одинаковой длины, отделяет назначаемое значение скобочкам, ясно видно что делает. Само собой понятия наглядности и крсоты чисто субъективны, но никаких причин использовать именно свойства заместо атрибутов также нет.)
__________________
29375, 35

Последний раз редактировалось Aetae, 03.08.2014 в 20:18.
Ответить с цитированием
  #5 (permalink)  
Старый 03.08.2014, 23:07
Аспирант
Отправить личное сообщение для Master_Sergius Посмотреть профиль Найти все сообщения от Master_Sergius
 
Регистрация: 29.07.2014
Сообщений: 42

Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )
Ответить с цитированием
  #6 (permalink)  
Старый 04.08.2014, 05:40
Аватар для Erolast
Профессор
Отправить личное сообщение для Erolast Посмотреть профиль Найти все сообщения от Erolast
 
Регистрация: 24.09.2013
Сообщений: 1,436

Есть одно простое правило - если код пишется долго и нудно, скорее всего, в нем что-то не так. Хороший код пишется быстро и легко. То есть, повторю Aetae, одинаковый функционал должен быть вынесен в функции, функции должны иметь понятное назначение и данные должны быть отделены от логики.
Ответить с цитированием
  #7 (permalink)  
Старый 04.08.2014, 09:00
Аватар для Aetae
Тлен
Отправить личное сообщение для Aetae Посмотреть профиль Найти все сообщения от Aetae
 
Регистрация: 02.01.2010
Сообщений: 6,590

Сообщение от Master_Sergius Посмотреть сообщение
Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )
Закон самообучения: видишь непонятное слово\аббривеатуру относящуюся к теме обучения - гуглишь и изучаешь. Вот и весь секрет. Иначе никуда не уедешь.
Учебник конкретно по js и на этом сайте отличный.
__________________
29375, 35
Ответить с цитированием
  #8 (permalink)  
Старый 04.08.2014, 09:19
Профессор
Отправить личное сообщение для WorM32 Посмотреть профиль Найти все сообщения от WorM32
 
Регистрация: 11.02.2014
Сообщений: 303

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. Про атрибуты уже говорили. Я бы тоже использовал свойства, а не атрибуты. Запись короче и понятнее.
Ответить с цитированием
  #9 (permalink)  
Старый 04.08.2014, 10:19
Аватар для Erolast
Профессор
Отправить личное сообщение для Erolast Посмотреть профиль Найти все сообщения от Erolast
 
Регистрация: 24.09.2013
Сообщений: 1,436

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

Последний раз редактировалось Erolast, 04.08.2014 в 10:22.
Ответить с цитированием
  #10 (permalink)  
Старый 04.08.2014, 10:51
Профессор
Отправить личное сообщение для WorM32 Посмотреть профиль Найти все сообщения от WorM32
 
Регистрация: 11.02.2014
Сообщений: 303

Сообщение от Erolast Посмотреть сообщение
Нет, это как раз правильно и это работает, но только в лисе. Значения параметров по умолчанию - часть еще неутвержденного ES6.
Это понятно, но вряд ли ТС писал данный код только под лису)
Ответить с цитированием
Ответ



Опции темы Искать в теме
Искать в теме:

Расширенный поиск


Похожие темы
Тема Автор Раздел Ответов Последнее сообщение
Как оптимизировать этот код, чтоб не повторяться?(JS, jQuery, HTML) Blondinka Элементы интерфейса 2 22.03.2014 12:59
Помогите оптимизировать этот код Gamestop Общие вопросы Javascript 3 20.01.2012 14:02
Прошу помочь. pro100(4eJI) Общие вопросы Javascript 13 02.01.2012 02:55
прошу помочь найти ошибку в самодельном drag&drop versoul Элементы интерфейса 1 03.03.2010 01:59
Прошу помочь с проверкой формы ввода Гость Общие вопросы Javascript 2 08.03.2008 12:10