03.08.2014, 19:36
|
Аспирант
|
|
Регистрация: 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
|
|
03.08.2014, 20:02
|
|
Тлен
|
|
Регистрация: 02.01.2010
Сообщений: 6,590
|
|
Сообщение от Rise
|
setAttribute заменить на свойство...
|
Зачем? Имхо setAttribute наглядней.
__________________
29375, 35
|
|
03.08.2014, 20:06
|
Профессор
|
|
Регистрация: 16.03.2010
Сообщений: 1,618
|
|
Master_Sergius, а если бы вопросов было не 5, а 105? Твой класс Questions занимал бы не 150, а 15000 строк кода?
|
|
03.08.2014, 20:10
|
|
Тлен
|
|
Регистрация: 02.01.2010
Сообщений: 6,590
|
|
Master_Sergius, для небольшого скрипта сойдёт. А так if'ы можно поменять на switch(а лучше объеденить одинаковые реакции в функции, а вопросы и ответы в отдельный массив, копипаст не рулит), именование функций с_нижним_подчёркиванием заменить на lowerCamelCase как принято в js. Также, для описания функций лучше сразу использовать jsdoc это поможет во множестве вещей, как то: работа ide, сжатие с помощью gcc, документация итд.
Сообщение от Rise
|
Чем наглядней не вижу преимуществ? Поясните.
|
Одинаковой длины, отделяет назначаемое значение скобочкам, ясно видно что делает. Само собой понятия наглядности и крсоты чисто субъективны, но никаких причин использовать именно свойства заместо атрибутов также нет.)
__________________
29375, 35
Последний раз редактировалось Aetae, 03.08.2014 в 20:18.
|
|
03.08.2014, 23:07
|
Аспирант
|
|
Регистрация: 29.07.2014
Сообщений: 42
|
|
Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )
|
|
04.08.2014, 05:40
|
|
Профессор
|
|
Регистрация: 24.09.2013
Сообщений: 1,436
|
|
Есть одно простое правило - если код пишется долго и нудно, скорее всего, в нем что-то не так. Хороший код пишется быстро и легко. То есть, повторю Aetae, одинаковый функционал должен быть вынесен в функции, функции должны иметь понятное назначение и данные должны быть отделены от логики.
|
|
04.08.2014, 09:00
|
|
Тлен
|
|
Регистрация: 02.01.2010
Сообщений: 6,590
|
|
Сообщение от Master_Sergius
|
Можно ещё ссылки на стайл гайды и всё такое? А то мне много терминов неясно ещё )
|
Закон самообучения: видишь непонятное слово\аббривеатуру относящуюся к теме обучения - гуглишь и изучаешь. Вот и весь секрет. Иначе никуда не уедешь.
Учебник конкретно по js и на этом сайте отличный.
__________________
29375, 35
|
|
04.08.2014, 09:19
|
Профессор
|
|
Регистрация: 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. Про атрибуты уже говорили. Я бы тоже использовал свойства, а не атрибуты. Запись короче и понятнее.
|
|
04.08.2014, 10:19
|
|
Профессор
|
|
Регистрация: 24.09.2013
Сообщений: 1,436
|
|
Цитата:
|
6. this.next = function (num=0) Это не работает. Правильно делать так.
|
Нет, это как раз правильно и это работает, но только в лисе. Значения параметров по умолчанию - часть еще неутвержденного ES6.
Последний раз редактировалось Erolast, 04.08.2014 в 10:22.
|
|
04.08.2014, 10:51
|
Профессор
|
|
Регистрация: 11.02.2014
Сообщений: 303
|
|
Сообщение от Erolast
|
Нет, это как раз правильно и это работает, но только в лисе. Значения параметров по умолчанию - часть еще неутвержденного ES6.
|
Это понятно, но вряд ли ТС писал данный код только под лису)
|
|
|
|