1. ActionScript / Говнокод #5381

    −105

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    16. 16
    17. 17
    18. 18
    19. 19
    20. 20
    21. 21
    22. 22
    23. 23
    24. 24
    25. 25
    26. 26
    27. 27
    28. 28
    29. 29
    30. 30
    31. 31
    private function handleBtnEvent(evt:MouseEvent):void
{
	
	switch(evt.currentTarget)
	{
		case exitBtn:
			break;
		
		case nextBtn:
			handleNextRounder();
			break;
		
		case reportBtn:
			handleReport();
			break;
		
		case hangUpBtn:
		case stopBtn:
			if(evt.currentTarget == stopBtn && _randomRoundStarted || 
				evt.currentTarget == hangUpBtn)
				handleHangUp();
			else 
				handleLeaveRandomRound();
				
			break;
	}
	
	enableMouseInteraction(false);
	TweenLite.delayedCall(1,enableMouseInteraction,[true]);
	
}

    Еще одно чудо народного зодчества от предыдущих ваятелей :)

    Запостил: wvxvw, 25 Января 2011

    Комментарии (11) RSS

    • ava gegMOPO4 25.01.2011 11:53 # 0

      Я не поспеваю за ходом вашей мысли. Может стоит указать, какие строчки не нравятся и как должно быть?
      Ответить

      • ava nibb13 25.01.2011 12:10 # 0

        Вот это выглядит сомнительно:
        enableMouseInteraction(false);
TweenLite.delayedCall(1,enableMouseInteraction,[true]);

        Для этих целей существует setTimeout.
        Ответить

      • ava wvxvw 25.01.2011 12:39 # −1

        ОК, абстрагируемся от конкретных названий и посмотрим, как все то же самое можно было записать более компактно и понятно.
        package tests
{
	import flash.display.Sprite;
	
	public class Test extends Sprite
	{
		public function Test()
		{
			super();
			
			this.testSwitch0(1, true);
			this.testSwitch0(2, true);
			this.testSwitch0(1, false);
			this.testSwitch0(2, false);
			trace("=======");
			this.testSwitch1(1, true);
			this.testSwitch1(2, true);
			this.testSwitch1(1, false);
			this.testSwitch1(2, false);
			trace("=======");
			this.testSwitch2(1, true);
			this.testSwitch2(2, true);
			this.testSwitch2(1, false);
			this.testSwitch2(2, false);
			trace("=======");
		}
		
		private function testSwitch0(condition:int, independentValue:Boolean):void
		{
			switch (condition)
			{
				case 1:
				case 2:
					if (condition == 1 && independentValue || condition == 2)
						trace("== 1 ==");
					else trace("== 2 ==");
			}
		}
		
		private function testSwitch1(condition:int, independentValue:Boolean):void
		{
			switch (condition)
			{
				case 1:
				case 2:
					if (!independentValue && condition == 1)
						trace("== 2 ==");
					else trace("== 1 ==");
			}
		}
		
		private function testSwitch2(condition:int, independentValue:Boolean):void
		{
			if (condition == 1 && independentValue || condition == 2)
				trace("== 1 ==");
			else trace("== 2 ==");
		}
	}
}


        Более того, свитч в этом месте не то что не нужен, он вреден. Нужно было каждому событию добавить по слушателю (что в приниципе и было сделано), и убрать эту братскую могилу вообще.
        Ответить

        • ava gegMOPO4 25.01.2011 21:16 # 0

          Это называется компактно и понятно?
          case hangUpBtn:
        handleHangUp();
        break;
case stopBtn:
        if(_randomRoundStarted)
                handleHangUp();
        else 
                handleLeaveRandomRound();
        break;
          Ответить

          • ava wvxvw 25.01.2011 21:46 # 0

            Я такое решение не предлагал, и, на мой взгляд оно не лучше оригинала т.как та же фунция будет фигурировать дважды. Опять же, если делать хорошо, то switch тут был не нужен (он вообще очень редко когда нужен... но это отдельный разговор), и я об этом уже сказал. А еще лучше было бы, если бы когда меняется _randomRoundStarted, подписывать stopBtn на handleHangUp или handleLeaveRandomRound, и вся эта конструкция оказалась бы не нужной.
            Говнистость этого кода, в моем понимании в том, что используя switch в этой функции мы проверяем дважды одно и то же условие, что можно избежать используя if-else например.
            Ответить

          • ava wvxvw 25.01.2011 21:56 # 0

            Хотя... не, в целом лучше :) ну и пусть будет один лишний раз функция написана, зато читать действительно понятнее, и в целом даже сгенеренный байткод будет короче :)
            Ответить

            • ava gegMOPO4 25.01.2011 23:23 # 0

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

              По поводу свитча против отдельных слушателей -- тут тоже могут быть разные соображения. Включая то, что придётся дублировать последние две общие строчки, и вырастет размер байткода (не знаю, как на ActionScript, а на Java, где функциональность делается через классы -- существенно). Да и компактнее один свитч в одном месте.
              Ответить

              • ava wvxvw 26.01.2011 02:09 # −1

                Во флеше одна кнопка (визуальный объект) перевесит десяток не-визуальных классов (в случае со слушателями тут как раз даже наоборот оптимизация получится, но даже если бы ее и не было...), так что "пожалеть" пару строчек на то, чтобы вместо кейса написать отдельную функцию - только лень / неряшливость / неопытность.
                Свитч неудобочитаемый, особенно ввиду того, что, например, дают вам через пару дней лог ошибки с номерами строк, а вы в этом классе делали небольшую правку - поди разбери в каком из кейсов была ошибка. А если это функция - то хоть куда ее передвинь, все равно потом найдется.
                Кроме того, повторно использовать кейс безо всей функции - ну никак :) А шансы того, что нужно будет во всех случаях какой-то общий фунционал примерно равны шансам, что в каждом случае нужен будет какой-то взаимоисключающий функционал (врезультате которого и появился, например, данный код).
                Ответить

                • ava bugmenot 26.01.2011 20:57 # +1

                  > функционал
                  детектед.
                  Ответить

                • ava gegMOPO4 27.01.2011 22:41 # 0

                  В Java каждый отдельный класс-слушатель съест дополнительно 0.5-1 Кб. Если таких обработчиков несколько сотен... Вот поэтому _иногда_ смысл в таком не очень красивом и поддерживаемом коде есть.

                  Если в ActionScript можно ссылаться без оверхеда прямо на конкретный метод -- тогда вопросов нет. Тем более, для каждого варианта (кроме stopBtn) уже предусмотрен отдельный метод.
                  Ответить

                  • ava wvxvw 27.01.2011 23:02 # −1

                    Не, во всех языках релизующих ECMAScript исполняемый код можно передавать по ссылке, и вообще не существует такого исполняемого кода, который нельзя передать по ссылке. Поэтому оверхед (и то очень сомнительный) в несколько строчек кода / несколько знаков, а может даже и нет.
                    А что javac не заинлайнит таких слушателей, это ж очевидные кандидаты?
                    Ответить

    Добавить комментарий