Как стать на правильный путь в написании кода?

Question

[Михаил Шатилов]

Я пишу код уже несколько лет, но последнее время меня начало уже даже не раздражать, а бесить качество своего кода. Самое обидное, когда ты даже не знаешь, как сделать лучше.
Например, модуль авиаперелетов (JS):

for (var i=0;i<len;i++) {
		if (data.Flight.hasOwnProperty(i)) {
			var segm_data = data.Flight[i].Segment;
			var suppliers = get_supplier(segm_data);
			//console.log(segm_data);return true;
			var flight_start_time = segm_data[0].Departure.attributes.Time;
			var flight_end_time = segm_data[0].Arrival.attributes.Time;
			var departure = segm_data[0].Departure.attributes;
			var arrival = segm_data[0].Arrival.attributes;
			//console.log(arrival);
			var dep_all_time = arrival.Date.split('-').reverse().join('-')+' '+arrival.Time;
			var arr_all_time = departure.Date.split('-').reverse().join('-')+' '+departure.Time;
			var flight_time = moment(dep_all_time).diff(arr_all_time,'seconds');
			if (
				(time2minutes(flight_start_time) < parseInt(this.filter.data.time.departure.start))
				|| (time2minutes(flight_start_time) > parseInt(this.filter.data.time.departure.end))
				|| (time2minutes(flight_end_time) < parseInt(this.filter.data.time.departure_back.start))
				|| (time2minutes(flight_end_time) > parseInt(this.filter.data.time.departure_back.end))
				|| flight_time > parseInt(this.filter.data.time.flight)*60
				|| !in_array_array(suppliers,this.filter.data.airlines)
			) {
				status = false;
				break;
			}
		}
	}

Это главный цикл, который отвечает за "попадание" элемента (перелета) в список на странице. Я вижу как одно из решений - бить все на мелкие функции, но хотелось бы услышать совета проф.

Comments

[Михаил Шатилов]

Еще пример UPD1 (обновляет параметры фильтра):

this.update_all = function() {
		if (flights.search_result.TotalCount == 0)
			return false;
        for (var i=0;i<flights.search_result.Air.length;i++) {
            if (flights.search_result.Air.hasOwnProperty(i)) {
				var item = flights.search_result.Air[i];
				this.transplants.process.call(flights.filter.transplants,item);
				flights.hot.process.call(flights.hot,item,i);
				for (var j=0;j<item.Flight.length;j++) {
					if (item.Flight.hasOwnProperty(j)) {
						this.airlines.process(item.Flight[j]);
					}
				}
            }
        }
		this.airlines.insert_html.call(flights.filter.airlines);
		this.transplants.insert_html.call(flights.filter.transplants);
	};

[Михаил Шатилов]

UPD2: Код несколько раз переписывался. Знаю про var, так же про неоднократные повторения segm_data[0]. Это возникло из-за смены поставщика данных.

Но больше интересно как бороться с кашей, я слышал про MVC, но я видел код, который может быть красивым без MVC.

Вот еще пример. Просто как бы все надо и все необходимо, но чувствую, что нужно изменить концепцию построения кода.
Еще пример:

this.refresh_content = function() {
        if (this.search_result.TotalCount == 0) {
			$('#search_res_items').html('');
			return false;
		}
        var items_content = [];
        usort(this.search_result.Air,function(a,b){
			//console.log(a);
			if (this.filter.data.sort == 'price') {
				if (a.Prices.Price.convert_value > b.Prices.Price.convert_value) {
					return 1;
				}
			} else {
				var dep_date = moment(a.Flight[0].Segment[0].Departure.attributes.Date+' '+a.Flight[0].Segment[0].Departure.attributes.Time);
				var arr_date = moment(a.Flight[0].Segment[0].Arrival.attributes.Date+' '+a.Flight[0].Segment[0].Arrival.attributes.Time);
				if (dep_date.diff(arr_date) < 0) {
					return 1;
				}
			}
			return 0;
		});
        for (var i=0;i<this.search_result.Air.length;i++) {
            if (this.search_result.Air.hasOwnProperty(i) && this.check_item_by_filter(this.search_result.Air[i])) {
				items_content.push(this.create_blocks.list_item_item(this.search_result.Air[i]));
            }
        }
        //$('#search_res_items').html(items_content.join(''));
		document.getElementById("search_res_items").innerHTML = items_content.join('');
    };

В настоящее время уже употребляю:

for (var i=0;i<this.search_result.Air.length;i++) { if (!this.search_result.Air.hasOwnProperty(i)) continue;

Но это лишь мелкая оптимизация. Проблемы со вложенностями были всегда.

[Михаил Шатилов]

UPD 3: воспользовался советами и сделал небольшие оптимизации типа уменьшения кол-ва var и дублирования лишних переменных.

Answer 1

[Петруша Укропов]

Я бы посоветовал оптимизировать строки

var flight_start_time = segm_data[0].Departure.attributes.Time;
var flight_end_time = segm_data[0].Arrival.attributes.Time;
var departure = segm_data[0].Departure.attributes;
var arrival = segm_data[0].Arrival.attributes;
var dep_all_time = arrival.Date.split('-').reverse().join('-')+' '+arrival.Time;
var arr_all_time = departure.Date.split('-').reverse().join('-')+' '+departure.Time;

заменив на

var departure = segm_data[0].Departure.attributes;
var arrival = segm_data[0].Arrival.attributes;
var flight_start_time = departure.Time;
var flight_end_time = arrival.Time;            
var dep_all_time = arrival.Date.split('-').reverse().join('-')+' '+flight_end_time;
var arr_all_time = departure.Date.split('-').reverse().join('-')+' '+flight_start_time;

Answer 2

[Андрей Буров]

У вас 4 раза используется segm_data[0], и нигде не используется просто segm_data
может тогда лучше сделать так:
var segm_data = data.Flight[i].Segment[0];
аналогично с flight_start_time и flight_end_time, к ним везде применяется time2minutes()

var flight_start_time = time2minutes(segm_data.Departure.attributes.Time);

А на какие маленькие функции вы бы хотели разбить этот кусок?

Answer 3

[Алексей Сундуков]

Вот меня смущает обилие var. Без такого количества дополнительных переменных реально ни как не обойтись? Учитывая отсутствие контекста другого кода бы рекомендовал для начала хорошо форматировать код. Например, так:

for (var i = 0; i < len; ++i)
{
	if ( !data.Flight.hasOwnProperty(i) ) {
		continue;
	}

	var
		  segm_data         = data.Flight[i].Segment
		, suppliers         = get_supplier(segm_data)
		, flight_start_time = segm_data[0].Departure.attributes.Time
		, flight_end_time   = segm_data[0].Arrival.attributes.Time
		, departure         = segm_data[0].Departure.attributes
		, arrival           = segm_data[0].Arrival.attributes
	;

	var
		  dep_all_time = arrival.Date.split('-').reverse().join('-')+' '+arrival.Time
		, arr_all_time = departure.Date.split('-').reverse().join('-')+' '+departure.Time
		, flight_time  = moment(dep_all_time).diff(arr_all_time,'seconds')
	;
	
	if (
		(time2minutes(flight_start_time) < parseInt(this.filter.data.time.departure.start))
		|| (time2minutes(flight_start_time) > parseInt(this.filter.data.time.departure.end))
		|| (time2minutes(flight_end_time) < parseInt(this.filter.data.time.departure_back.start))
		|| (time2minutes(flight_end_time) > parseInt(this.filter.data.time.departure_back.end))
		|| flight_time > parseInt(this.filter.data.time.flight)*60
		|| !in_array_array(suppliers,this.filter.data.airlines)
	) {
		status = false;
		break;
	}

}

Answer 4

[Николай Васильчук]

Уменьшайте вложенность.

// Было
for (var i=0;i<len;i++) {
    if (data.Flight.hasOwnProperty(i)) {
        // ...
    }
}
// Стало
for (var i=0;i<len;i++) if (data.Flight.hasOwnProperty(i)) {
    // ...
}

// Было
for (var i=0;i<flights.search_result.Air.length;i++) {
    if (flights.search_result.Air.hasOwnProperty(i)) {
        // ...
    }
}
// Стало
for (var i=0;i<flights.search_result.Air.length;i++) {
    if (!flights.search_result.Air.hasOwnProperty(i)) continue;
    // ...
}

Comments

[gro]

Почему минусов здесь нельзя ставить?

[Михаил Шатилов]

@gro А что не так, вроде неплохо читается?

[Николай Васильчук]

@gro Поясните что не так?

[gro]

Плохо или не плохо читается, это дело вкуса и спорить об этом можно до одурения. А чтобы люди не спорили о чепухе и занимались более важными вещами есть стандарты кодирования. Их и придерживаться надо.

[Николай Васильчук]

@gro Ссылку на стандарт, который нарушает этот код вы, наверное, не нашли?

[gro]

jshint и jslint единственные на данный момент "стандарты" в js-сообществе.

[gro]

а то, что любой блок должен обрамляться фигурными скобками, даже если там одна инструкция, это не имеет смысл обсуждать для любых c-подобных языков.

Answer 5

[Par Mactep]

Как-то много сущностей вы плодите. Может есть смысл их заворачивать в обьекты...
Исходя из количеству циклов могу посоветовять нечто наподобие each из underscore.js

Answer 6

[Николай Васильчук]

Вот вам еще оптимизация. Посмотрите на этот код (у вас много подобного):

for (var i=0; i<arr.length; i++) {
    if (!arr.hasOwnProperty(i)) continue;
    // ...
}

Во-первых, цикл. При обходе массива лучше делать так, будет быстрее:

for (var i=0, l=arr.length; i<l; i++) {
    // ...
}

Или так:

var l = arr.length;
while (l--) {
    // ...
}

Во-вторых, зачем вы проверяете arr.hasOwnProperty(i)? Вы обходите массив последовательно от 0 до arr.length-1, то есть если индексы в массиве от 0 до arr.length-1, то можно не проверять, вы обойдете все элементы, а если индексы проставлены иначе, то вы обойдете не весь массив и ваш код не работает как надо. Такую проверку обычно делают при обходе объекта с помощью for ... in ...