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

[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]

Object(this).root.
- что вот этот кусок значит, по вашему? Почему он именно такой?

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

Добавлено через 5 минут
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 я бы попозже разбирать советовал только.

[expl]

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

[Korchy]

И ни одного комментария в коде.

[Aquahawk]

И ни одного комментария в коде.
В столь малом коде и не должно быть комментариев имхо

[caseyryan]

В столь малом коде и не должно быть комментариев имхо
А разве присутствие комментариев как-то соотносится с размером кода?
Комментарии должны быть в любом коде, который в последствии может вызывать сомнения.
Посмотрев на этот код, я бы сказал, что их специалист еще очень вежливо отписался.
Если честно, код просто ужасен.
Я бы тоже отказал соискателю с таким кодом. Но почему, тут уже до меня всё сказали.

п.с. Тема-то флеймовая. Странно, что ее до сих пор не перенесли

[Aquahawk]

Этот код будучи правильно сделан и написан не должен требовать комментов. Я сторонник того что коммент должен пояснять то что, что может быть воспринято как ошибочно написанный код, чтобы читающий человек понял что это написано сознательно. Сюда относятся присваивания в if, хотя я не пишу так, сюда же относится fallthrough, прочее.
Вот, например, метод очистки модели данных в котором я не так давно написал коммент.
/**
* @private
*/
protected override function clear(event:Event=null):void {
this._food = null;
this._money = null;
this._premium = null;
this.data = null; // сеттер вызван намеренно

super.clear(event);
}
в результате читающий не подумает переправить data на _data, потому что есть комментарий, и пойдёт сеттер посмотрит, и увидит что в нём происходит отписка слушателей от имеющейся даты и переподписка их на новую если она есть. Таким образом подписка отписка неразрывно связана с наличием данных, что важно.

Добавлено через 2 минуты
А комметировать код в духе:
// это таймер
// подписали лисенера

// вункция лисенер, принимает параметром ивент потому что её подпишут на таймер
я считаю крайне некорректным

[mooncar]

п.с. Тема-то флеймовая. Странно, что ее до сих пор не перенесли
Тема профильная, о качестве кода AS3. Потому и остается здесь.

[caseyryan]

А комметировать код в духе:
Цитата: // это таймер
// подписали лисенера

// вункция лисенер, принимает параметром ивент потому что её подпишут на таймер

я считаю крайне некорректным

Никто о таком комментировании не говорит. Это, естественно, бессмысленно.
Я говорю о коде, котоырй может вызывать сомнения или затруднения в понимании в будущем.
Код из первого сообщения явно требует хотя бы пары комментов.

ну, это, в общем-то как кому нужно
Вот так писать:

this._food = null;
this._money = null;
this._premium = null;

по сути, тоже не целесообразно. Если писать приватные поля с андерскором, то указывать this тоже нет никакой необходимости. И так понятно, что это относится к this. И компилятору это понятно.

Тема профильная, о качестве кода AS3. Потому и остается здесь.
Ну, во всяком случае, заголовок темы не говорит ничего о качестве написания ас3 кода

[artcraft]

-De- и alatar всё очень правильно написали
даже из этого маленького кусочка кода видно что это спагетти-код

[Aquahawk]

по сути, тоже не целесообразно. Если писать приватные поля с андерскором, то указывать this тоже нет никакой необходимости.
У нас это просто стандарт. Тем более что с андерскором идут приватные и protected, и при обращении к протектед полям предка пишем super._food = null; Для явного подчёркивания того что и где.

[expl]

Про комментировать/некомментировать поговорили, про this/_ упомянтули.
Еще про основное надо обяснить оппонентам что они неправы:
- пробелы vs табы
- страшный_вред vs неоспоримая_польза венгерской нотации