Как объединить функции слайдера по таймеру и по клику?

Question

[andreyqin]

Есть 2 функции: одна сменяет баннеры по таймеру, другая - при клике на переключатели.

Хотелось бы объединить эти функции в одну, но не знаю, как. Сам код:

var activeIcon = '/images/circle_active.png',
			inactiveIcon = '/images/circle_inactive.png',
			interval = 5000;

		   // Функция смены баннеров по таймеру
		  function slider() {
			var current = $('.scrollbar img[src="' + activeIcon + '"]').closest('span'),
				next = (current.next().length > 0) ? current.next() : $('.scrollbar span:first-child'),
				$banner = $('.banner img'),
				background = $banner.attr('src'),
				src = next.data('src'),
				id = next.data('id');
			$('.scrollbar img').attr('src', inactiveIcon);
			next.find('img').attr('src', activeIcon);
			$('.banner').css('background', 'url(' + background + ')');
			$banner.fadeOut(100, function() {
				$(this).attr('src', src).data('id', id).fadeIn(800);
			});
		  }
		  var changeBanners = window.setInterval(slider, interval);

		  // Смена баннеров при клике на кнопку
		  $('.scrollbar').on('click', 'span', function() {
			var $this = $(this),
				$banner = $('.banner img'),
				background = $banner.attr('src'),
				src = $this.data('src'),
				id = $this.data('id');
			clearInterval(changeBanners);
			$('.scrollbar img').attr('src', inactiveIcon);
			$this.find('img').attr('src', activeIcon);
			if (src != $banner.attr('src')) {
				$('.banner').css('background', 'url(' + background + ')');
				$banner.fadeOut(100, function() {
					$(this).attr('src', src).data('id', id).fadeIn(800);
				});
			} else {
				return false;
			}
			changeBanners = window.setInterval(slider, interval);
		  });

Может кто-нибудь помочь или подкинуть идею?

Answer 1

[Юрий Горин]

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

Comments

[andreyqin]

По сути, этот кусок вот этот:
$('.banner').css('background', 'url(' + background + ')');
$banner.fadeOut(100, function() {
$(this).attr('src', src).data('id', id).fadeIn(800);
});

То есть, только его выделять в отдельную функцию?

[Юрий Горин]

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

[andreyqin]

Понял. То есть, это все, что можно сделать в данном случае для улучшения?

[andreyqin]

Упрощенный вариант - jsfiddle.net/Gg5XU . Что скажите?

[Юрий Горин]

@andreyqin Детально не вчитывался, а так значительно лучше, чем было.

Answer 2

[Юрий Лобанов]

А зачем Вам объединять разные функции?

Comments

[andreyqin]

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