Ревью тестового задания

[Фенёк]

Ну пулов у тебя нет.

Равно как и нет часто удаляемых и создаваемых объектов. Для чего мне пул? Ну то есть, серьезно, для каких целей его предлагается использовать? Нет, я мог бы конечно создать пул и не пользоваться им, только зачем?

ScreenPoint и IsoPoint это крутые классы, но зачем, когда есть старый добрый Point?

На мой взгляд есть очень веская причина - читаемость кода. Человек читает и сразу понимает где изомерические координаты, а где экранные. К тому же в случае когда этих поинтов понадобится много, они будут занимать меньше пространства в памяти, потому что у них нет лишних методов и свойств.

Судя по предыдущей открытой тобой теме ты занимался вовсе не тем, о чём тебя просили. Задача была не про то как запихнуть прямоугольники, а про то как ты умеешь работать с изометрией.

Да, есть такое дело, одолело пристрастие к деталям, но если вопрос был конкретно об изометрии - она работает. Сортировка производится правильно (хотя сегодня я понял, что проверка на существование ребра графа могла быть примерно 4мя строчками короче, что, правда, не умаляет ее корректности), координаты рассчитаны правильно

Честно, если бы я принимал программиста на такую зарплату, как указано в вакансии, я тоже бы тебя не взял.

Чтобы добавить немного справедливости, скажу, что цена вопроса с моей стороны была на треть ниже.

Вобщем, дабы не усложнять всем жизнь пройдусь по тем комментариям, к которым я могу предоставить код.

алгоритмически не эффективная сортировка

Построение ребер графа всегда требует полного перебора

Изометрическая сортировка реализована некорректно. Прямоугольные объекты были не просто так - с ними нельзя использовать элементарную сортировку по глубине, т.к. возникнут артефакты в определенных случаях. Решениями являются к примеру использование топологической сортировки, попиксельный z buffer или нарезание объектов на вертикальные полосы сортирующиеся индивидуально

Наглейшая ложь человека, даже не открывавшего код. Привожу реализацию

		private function sortChild(container:DisplayObjectContainer, child:IsoObject):void
		{
			for (var i:int = 0, length:int = _model.buildingsCount; i < length; i++)
			{
				var compareBuilding:Building = _model.getBuildingAt(i) as Building;
				var currentIsNearest:Boolean = false;
 
				if (child != compareBuilding)
				{
					if (child.isoLeft >= compareBuilding.isoRight && child.isoTop >= compareBuilding.isoBottom)
					{
						currentIsNearest = true;
					}
					else if (Projection.intersects(child.isoLeft, child.isoRight, compareBuilding.isoLeft, compareBuilding.isoRight) &&
						child.isoTop >= compareBuilding.isoBottom)
					{
						currentIsNearest = true;
					}
					else if (Projection.intersects(child.isoTop, child.isoBottom, compareBuilding.isoTop, compareBuilding.isoBottom) &&
						child.isoLeft >= compareBuilding.isoRight)
					{
						currentIsNearest = true;
					}
 
					if (currentIsNearest && container.getChildIndex(child) < container.getChildIndex(compareBuilding))
					{
						container.swapChildren(child, compareBuilding);
					}
				}
			}
		}

Повторюсь, что да, она могла быть несколькими проверками меньше, но с математической точки зрения, она корректна

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

Такая же наглая ложь, свидетельствующая о том, что человек не потрудился даже посмотреть

код составление чанка карты

		private function createChunk():QuadBatch
		{
			var chunk:QuadBatch = new QuadBatch();
			var tile:Image      = new Image(LandscapeAssetManager.instance.getTexture("landscapeTiles_081.png"));
			tile.alignPivot(HAlign.CENTER, VAlign.TOP);
 
			for (var col:int = 0; col < _model.CHUNK_SIZE_IN_TILES; col++)
			{
				for (var row:int = 0; row < _model.CHUNK_SIZE_IN_TILES; row++)
				{
					var screenPoint:ScreenPoint = IsoTransformer.isoToScreen(col * _model.TILE_SIZE, row * _model.TILE_SIZE);
					tile.touchable = false;
					tile.x = screenPoint.x;
					tile.y = screenPoint.y - _model.TILE_HEIGHT;
					chunk.addImage(tile);
				}
			}
 
			return chunk;
		}

Ни о каких спрайтах здесь и речи нет. Все пакуется в квадбатч и ,соотвественно, потом отправляется одним вызовом на отрисовку.

Нелогичная работа с изометрическими размерами, координатами, конвертированием между screen и iso space

А вот тут человек выдал в себе ПОЛНОГО профана в области математики, опять-таки, привожу код

package util
{
	public class IsoTransformer
	{
		private static const ALPHA_RAD:Number = Math.atan(0.5);
		private static const TAN:Number       = Math.tan(ALPHA_RAD);
		private static const SIN:Number       = Math.sin(ALPHA_RAD);
		private static const COS:Number       = Math.cos(ALPHA_RAD)
 
		public static function screenToIso(x:Number, y:Number):IsoPoint
		{
			var isoX:Number = (x * TAN + ((y - x * TAN) / 2)) / SIN;
			var isoY:Number = ((y - x * TAN) / 2) / SIN;
			return new IsoPoint(isoX, isoY);
		}
 
		public static function isoToScreen(isoX:Number, isoY:Number):ScreenPoint
		{
			var x:Number = isoX * COS - isoY * COS;
			var y:Number = isoX * SIN + isoY * SIN;
			return new ScreenPoint(x, y);
		}
	}
}

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

Но меня просто взбесила логика в духе "не понимаю как работает - значит неправильно".

отсутствие кэширований и отсечений

Если я правильно суть претензии, под кэшированием человек понимал составление квадбатчей. Если под отсечением понималось прекращение отрисовки объектов, вышедших за пределы видимости, то опять-таки, вывод напрашивается о том, что человек ничего не прочел.

код:

		public function moveBy(xOffset:Number, yOffset:Number):void
		{
			x += xOffset;
			y += yOffset;
 
			for (var i:int = 0, length:int = _model.buildingsCount; i < length; i++)
			{
				var building:Building = _model.getBuildingAt(i);
				var localX:Number     = x + building.x;
				var localY:Number     = y + _buildingsLayer.y + building.y;
 
				var old:Boolean       = building.visible;
				building.visible =
					(localX + building.screenLeft > 0) &&
					(localX + building.screenRight < _viewportWidth) &&
					(localY + building.screenBottom > 0) &&
					(localY + building.screenTop < _viewportHeight);
			}
		}

Странные архитектурные решения вроде менеджера ресурсов под каждый отдельный атлас

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

Из серии "я так не делаю - значит неправильно"

Полностью отсутствует ощущение, что проект можно расширять: вбитые названия и отсутствие возможности разнообразить карту.

Для тестового задания собранного за два дня? Серьезно?

Вобщем, поверьте мне, я целиком и полностью признаю, что я далеко не гуру разработки, и мне конечно же есть чему учиться и куда стремиться, но черт возьми, киньте в меня камень, если ХОТЯ БЫ ОДИН из этих комментариев справедлив.

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

Кстати да, Rzer, большое спасибо, что нашли время заглянуть внутрь кода.