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

[Awesomni]

Друзья, недавно, интереса ради, пытался сменить работу и в качестве собеседования у меня попросили посмотреть мой код.
Я выслал работодателю следующий класс:

package obstacles
{
	import flash.display.MovieClip;
	import flash.utils.Timer;
	import flash.events.TimerEvent;
	import PointsConfig;
 
 
	public class Sankship extends MovieClip implements IObstacle
	{
 
		private var t:Timer = new Timer(350,0);
		private var e3:Boolean;
 
		public function Sankship()
		{
			e3 = Object(this).root.getGameState() == "3";
			if ((Object(this).root.artefact == 2) && e3)
			{
				t.start();
				t.addEventListener(TimerEvent.TIMER, checkCrush);
			}
		}
		private function checkCrush(e:TimerEvent):void
		{
			if (e3)
			{
				if (this.hitTestObject(Object(this).root.ship_mc))
				{
					Object(this).root.scorepoints +=  PointsConfig.SANKSHIP;
					Object(this).root.scoreboard_mc.update();
					this.dispose();
				}
			}
		}
		public function dispose():void
		{
			t.stop();
			if (t.hasEventListener(TimerEvent.TIMER))
			{
				t.removeEventListener(TimerEvent.TIMER,checkCrush);
			}
			this.parent.removeChild(this);
		}
	}
 
}

И ответ был следующим:

Уважаемый Николай!
К сожалению, мы пока не готовы предложить Вам свою вакансию.
Ниже комментарий нашего специалиста:

"Кода мало, и то что есть - средне. И даже могу точно сказать, что если бы мы так писали, то поддерживать приложения было бы почти невозможно".

Объясните, пожалуйста, в чём мои ошибки и как их исправить?

[udaaff]

http://www.flasher.ru/forum/showthread.php?t=138349

[-De-]

Object(this).root.

- что вот этот кусок значит, по вашему? Почему он именно такой?

[Awesomni]

Сообщение от -De-

Object(this).root.

- что вот этот кусок значит, по вашему? Почему он именно такой?

Я не знаю другого способа обратиться к самому первому ролику, к прародителю всех роликов..
Подскажите, как я должен был поступить?

Добавлено через 5 минут

Сообщение от -De-

Object(this).root.

- что вот этот кусок значит, по вашему? Почему он именно такой?

Понял, Вас смутило приведение к типу Object. Почему я так делаю, я не знаю.

[Krusty]

Судя по комментарию- как минимум, за наличие магических чисел.

[КорДум]

Архитектура приложения, откуда выдран класс, очевидно неверная. Никаких обращений к родителям. Только родители решают, что им делать с подопечными.

[alatar]

1. Ничего не значащие имена переменных (назовите их нормально). Вместо "t", что-то типа updateTimer, хотя не понятно что он вообще делает в этом классе.
2. "Магические числа". Заведите константы для состояний и не сравнивайте числа как строки.
3. Постоянные обращения к вышестоящим объектам. Это нарушение принципа инкапсуляции.
4. Объект сам себя удаляет.

Подскажите, как я должен был поступить?

Нет никакой необходимости дергать родителя напрямую. game state и artefact можно было передать в конструктор объекта, а checkCrush должен проверять вышестоящий (или специальный) класс.

[maxkar]

> Объясните, пожалуйста, в чём мои ошибки и как их исправить?
Кода действительно мало. И то, что есть, плохое.

1. Обращение к объектам с логикой через DisplayList (это все ваши Object(this).root). А вдруг ваш root будет потом вложен в другой контейнер (мало ли, как изменятся требования)? Нужные объекты нужно через конструктор передавать и работать напрямую.
2. Вашему Sankship совершенно необязательно наследовать MovieClip. Он может создавать нужный клип и кто-то (он сам или пользователь класса) будет добавлять ui в список отображения. А сам класс останется контроллером.
3. Не понятно, кто такой IObstacle и зачем его implements ваш класс. Вроде бы методов из интерфейса нет. А маркер-интерфейсы обычно не нужны.
4. Совершенно не понятно, кто такое e3. Серьезно. Название должно отображать его логику.
5. Кто такие "2" и "3" - тоже не понятно. Это должны быть именованные константы. Литералы допустимы только в "натуральном" контексте. Это математика (там уместны числа, когда их назначение более-мене ясно). И строковые литералы для случаев, когда их содержимое явно обозначает назначение (т.е. константа звалась бы примерно так же, как сам литерал).
6. Самоубийство из DisplayList. Это плохо и, вероятно, не нужно. Удалять должен пользователь класса. Или сам класс, если он является контроллером и имеет ссылки на контейнер и UI.
7. Зачем у вас проверка t.hasEventListener(TIMER)? Она не нужна. removeEventListener вроде бы не ругается, если слушатель не существует. А вот у removeChild там же никакой проверки нет, хотя как раз parent может быть null (мало ли, кто еще ваш объект удалил из display list). К тому же dispose является публичной, так что может быть вызвана несколько раз (один раз из вашего обработчика таймера, например).
8. В checkCrush лично мне не нравится большая вложенность. if(!e3) return;
9. Там же какие-то поползнования и вызовы методов через display list. См. п. 1. Да и объект для hitTestPoint можно передавать в конструкторе, а не доставать из display list.

Добавлено через 2 минуты
> Понял, Вас смутило приведение к типу Object. Почему я так делаю, я не знаю.
Ну так разберитесь, зачем вы это делаете. Вы должны быть в состоянии объяснить любое (абсолютно любое) решение в коде (выбор архитектуры/дизайна, выбор структур данных и алгоритом, выбор конкретных действий в коде). Программист должен хорошо понимать, как работает программа. Иначе нет никаких гарантий, что она вообще работает.

[Awesomni]

Спасибо всем за ответы. Буду работать над собой.

[-De-]

Не пишите то, что не знаете как работает. Давайте нормальные имена переменным. Не используйте магических чисел.
Вот для начала.
К Object лучше вообще не приводить, это "я не знаю, что это за штука и молюсь, чтоб у неё было то, к чему я обращаюсь". this.root - это ваш Document Class, к нему приводите. Ну и вообще советов уже понадавали) Про MVC я бы попозже разбирать советовал только.