Блог разработчика

отказали вероятнее всего по “политическим” мотивам – из серии “чувак с таким опытом на джуниорской позиции будет себя чувствовать психологически некомфортно” или “нихренасе, та его надо тимлидом вместо меня брать. не-не-не…”

На самом деле просто достаточно было попросить список замечаний.

1. Комментарии в коде не нужны. Код должен быть написан таким образом, чтобы его не нужно было комментировать. Если код нуждается в комментариях – значит он нечитабелен. Это касается абсолютно всех комментариев
2. Нарушение соглашений об именованиях переменных. Названия t, _rnd, массивы с именем ‘array’ и т.д. не соответствуют соглашениям об именовании переменных в .net. Префиксы не нужны, сокращения тоже (исключения – если они общепринятые, например ui, xml).
3. Логика работы перемешана с вводом-выводом. В реализации очереди и в тестах присутствует консольный вывод. Это антипаттерн, его появление сводит возможность переиспользования кода к нулю
4. В тестах используется рандомный генератор. Смысла от подобных тестов при рефакторинге нет, поскольку единажды случайно воспроизведенная ситуация может никогда не повториться в будущем. Сам факт необходимости введения случайных величин говорит о том, что это функциональные, а не модульные тесты
5. define в С# использовать крайне не рекомендуется. Это не C и C++, дефайны в дотнет очень сильно понижают читабельность кода
6. Magic numbers antipattern. Эта проблема присутствует также абсолютно во всех тестах, кроме того также есть захардкоженные литералы, назначение которых неочевидно («t»). Magic numbers необходимо выносить в константы с именами, сообщающими об их назначении
7. Тесты проверяют более одной ситуации. Разработка современных модульных тестов подразумевает следование принципам F.I.R.S.T. и AAA
8. Неверная проверка исключений в тестах. Тестирование с использованием NUnit подразумевает использование соответствующих конструкций (например Assert.Throws или соответствующих атрибутов)
На этом можно остановиться. Дальнейший углубленный анализ кода на данном этапе не имеет смысла.
Резюме: стилистика кода в целом соответствует С++, но не высококачественному коду на C#. Основная цель задания – не проверить способность кандидата реализовать поставленную задачу, которая в данном случае является простейшей, а умение разрабатывать очень качественный код и знание основ современного тестирования в рамках .NET.

Я приношу свои извинения если мы вас чем-либо обидели. Но все что требовалось, это – основы современной разработки ПО; для того чтобы правильно выполнить задание необходимы лишь знание самых основных подходов, библиотек и инструментов; никаких специфических библиотек или навыков не требуется. Стиль и качество кода, прозрачность архитектурных решений и алгоритмов является приоритетом современной разработки, когда код, написанный одним программистом, читают тысячи других.
В итоге вам отказали исключительно из тех соображений, что в данном случае это лучше для вас. У вас есть богатый опыт самостоятельной разработки на C++, но работе в команде на C# вас необходимо готовить практически с самого начала.
Мы не против такого варианта развития событий, но поскольку вы расчитываете на что то более, чем начинающий разработчик, то скорее всего вы сможете найти более интересную для вас работу.

От себя еще добавлю: никогда не слушайте людей, которые вам говорят “у вас все нормально, я тоже так пишу. Они к вам придрались”. Самолюбие у программиста очень важно, но оно не должно быть препятствием к развитию. Критика – самая лучшая мотивация к действию. Рекомендую больше читать, писать, изучать, общаться с опытными разработчиками, проводить работу над ошибками – и все у вас получится!

Roman, да нет щаровара живет себе потихоньку потихоньку. Просто хочется чего-то новенького, развиваться как-то..

И еще, у меня такое впечатление что какой-то сильно формальный, чуть ли не автоматический подход к анализу задания. То есть найдено куча пунктов – “минусов”, но похоже особо без раздумываний относительно веса каждого конкретного пункта.

Вы извините, но очень похоже на плохой вариант вот отсюда http://www.rsdn.ru/forum/dotnet/4873843.aspx?tree=tree 🙂

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

>>>Еще рекомендую обратить внимание на то, как вы реагируете на критику вашего кода. Вам было в первую очередь обидно, а не интересно

Да конечно обидно. Сейчас попробую обьяснить почему – ради написания этого задания я фактически выучил C#, ну по крайней мере синтаксис и азы, а так же часть фреймворка, например работу с потоками. Я узнал что такое блокирующая очередь, что она должна делать и как ее можно реализовать. Я над этим кодом работал дня три – читал, думал, проектировал, писал, отлаживал, тестировал. Я готов обосновать каждую строчку в коде очереди – иначе бы ее там не было. И тут вдруг я получаю вердикт что “код плохой”, без всякой аргументации. Хотя я, скромно, считаю что код очереди вполне нормален, тесты – да, согласен – там и код хуже гораздо, и избыточности много и вообще не соответствует правильному подходу. Но я сразу предупредил, что тесты я в жизни не писал еще. И у меня большое подозрение что человек смотревший код не сильно вникал в сутуацию, а ответил на основании формальных критериев (увидев префиксы, дефайн, переменную array и т.д.). Да мне было в первую очередь обидно 🙂

И сейчас когда мне повезло, вы зашли на мой блог и огласили список недостатков. Все равно вопросов больше чем ответов.
Меня действительно немного разочаровала фраза “По поводу замечаний – в большинстве пунктов спорить с вами не буду,”. Потому что мне действительно интересно узнать что на ваш взгляд было бы уместнее в моем коде вместо использования #define или услышать что все таки дефайн там разумен. Точно так же про наименования переменных – интересно услышать пару конкретных примеров (ну кроме _to каюсь не лучшее имя) – где я нарушил стандарт кодирования который вы дали. И по поводу консольного вывода я до сих пор в недоумении – 1. вы утверждаете что он у меня есть в коде очереди. 2. вы утверждаете что его присутвие сводит возможность использования кода к нулю. И мне действительно интресно было бы получить комментарий к моим ответам по поводу этих пунктов….

>>Но особо важные и критичные – #3, #6, #7.
#3й это там где меня несправедливо упрекнули в наличии консольного вывода в классе очереди, которого нет? И в том что консольный вывод в тестах не дает их использовать? Несмотря что вывод в тестах предназначен только для облегчения понимания происходящего начинающим программистом (типа меня) желающим разробраться как оно внутри работает, и абсолютно не влияет на логику работу или прохождение тестов. Просто еще один дополнительный уровень контроля если что.

#6. Magic numbers antipattern .Формально вы правы. Фактически все magic numbers локализованны в пределах одной функции, вот положа руку на сердце – учитывая крайне узкую локализацию использования даных чисел, и желательность настройки числа для каждой процедуры – не вижу особого смысла выносить их на уровень класса. К тому же я все таки подходил к этому коду как к тестовому заданию, а не как к коду который будет читатся и сопровождатся много лет в продакшене. Но формально да, вы правы – magic numbers это плохо.

#7 Да, тут я согласен, тесты написаны очень плохо. Но я предупреждал что с тестированием я абсолютно незнаком… что вы хотите – я меньше чем за день освоил азы тестирования,написал и отладил функциональные тесты для очереди – мне даже не очень стыдно, хоть я и понимаю что тесты плохие:)

>>>Однако Junior разработчик в нашем понимании – человек с малым опытом работы, но отлично подготовленный в теории; знающий, но не всегда умеющий применять best practices и паттерны проектирования. Согласитесь, никаких специфических знаний и библиотек не требуется, тестовое задание – простейшее, которое можно было придумать для охвата минимально необходимого набора знаний.
Согласен задание не сложное, разве что тесты смущают. А может вы можете, в порядке исключение, прислать мне в приват, 1-2 тестовых задания от джуниора которые прошли вашу проверку? Действительно интересно посмотреть.

>>>А в отношении сложившейся в целом ситуации: не переживайте и не расстраивайтесь! Очень часто бывает, что работодатели отказывают. Этому может быть миллион причин, а вовсе не значит что работодатели плохие, или “вы займете чье-то место” и т.п. Особенно часто такое случается с опытными разработчиками – девелопер с определенными знаниями и навыками не подходит под конкретную вакансию на проект, с учетом всех особенностей проекта и разработчика. Коммерческая разработка – это не только написание кода; это – сложный процесс взаимодействия людей различных профессий и квалификации.

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

Но все же это было ясно до того как мне дали тестовое задание, зачем было его давать, если было изначально понятно что я не подхожу по параметрам? И главное зачем было мотивировать отказ формулировкой “негодный код”, задевая мое самолюбие?:)

1. Плохие имена сущностей
Имена должны соответствовать требованиям и максимально точно сообщать назначение переменной. Это нужно для самодокументирования кода.
Примеры плохих имен:
class BegBlockedQueue2
Префикс ‘Beg’ вероятно сообщает о вашем псевдониме. Это недопустимо в коде production качества. Постфикс ‘2’ тоже не нужен – он не дает информации о назначении сущности. По сути нужно было реализовать только одну блокирующую очередь. Если уж реализованы две – то имена должны сообщать об особенностях реализации.

private readonly Queue _data
Коллекции должны иметь постфикс ‘s’ (множественное число по английски). Хорошее имя было бы _items, _elements или что-то подобное.
В тестах: имена _to (сокращение), _rnd (сокращение), array (когда нужно существительное во множественном числе), tAdd, tTake. Венгерка не используется в современном C++, а в C# – тем более.
В Program.cs (вообще это не нужно было реализовыать): Test1, Test2, q.

2. Мультитредность
public int Count
{
get { return _data.Count; }
}
Несинхронизированный доступ к разделяемой коллекции. То, что это лишь чтение, а не запись, не дает нам права так делать – мы не знаем как именно это свойство реализовано.
Чтобы понять о чем я – вспомните mutable поля из C++, которые могли изменяться при доступе к константным членам-методам класса. В результате метод, который только читает, может на самом деле вносить изменения.

3. Кодировать нужно в едином стиле. Если мы используем auto properties, то их надо использовать везде:

public bool IsAddingCompleted
{
get { return _isAddingCompleted; }
protected set { _isAddingCompleted = value; }
}

public int BoundedCapacity { get; protected set; }

В этом участке кода есть еще пару нюансов: поскольку наш объект поддерживает многопоточность – в свойствах нет синхронизации чтения и записи этих значений.
Второй нюанс: нарушение инкапсуляции. Protected поля и свойства можно делать только в случае если есть наследование, и наследник использует эти свойства/методы. Если наследников нет и в будущем не ожидается – protected доступ не нужен.

4. Сигнатура метода TakeInThread должна быть объявлена с параметром типа int:

void TakeInThread(object data)
А ниже:
if ((int)data > 0) Thread.Sleep((int)data);
В C# так делать не принято.
Чтобы избежать необходимости каста в методе – можно использовать лямбда-выражения при создании потока. В стиле:
var executor = new Thread(o => TakeInThread((int)o);
…
void TakeInThread(int sleepingTime)

5. Неверно отрабатываются и нет тестирования альтернативных путей исполнения
В конструкторе нет валидации boundedCapacity, в результате возможно создание очереди с boundedCapacity = 0. Это явный недостаток, приводящий к потенциальным алгоритмическим ошибкам. Кроме того такая ситуация не тестируется.

6. #define крайне нежелательны и в современном C++, так как они глобальны, очень плохо отслеживаются и контролируются. В C# дефайны недопустимы, им всегда есть более удачная замена.

7. MagicNumbers. Их не надо выносить на уровень класса. Достаточно объявить их локальными константами. К примеру вместо:

int[] array = new int[20];
нужно написать так:
const int TestAddElementsCount = 20;
int[] array = new int[TestAddElementsCount];

8. Конструкции ++t использовать нежелательно. Гайдлайны C# рекомендуют использовать постфиксную форму инкремента как более читабельную. Тем более, в отличие от C++ в C# .Net это производительности не добавляет.

9. Консольный вывод в тестах.
Да, в очереди действительно нет консольного вывода. Но сути это не меняет: в тестах консольный вывод недопустим даже для отладки. Для отладки есть дебаггер, для логирования – логгер. Консоль нужна для сообщения пользователю. Тесты (любые: модульные или функциональные) должны оформляться как ClassLibrary и писаться таким образом, что тест своим именем сообщает о проблеме. Также тест имеет 2 состояния: прошел или не прошел. Дополнительная отладочная информация (если она необходима) должна показываться через средства NUnit. В текущем задании такие ситуации, где такая информация необходима, придумать сложно. Использование консоли может повлиять на ход теста, а этого допускать нельзя.

Про тесты вроде все выяснили.

Думаю этого достаточно, чтобы понять, что код очень далек от идеала.

По причинам отказа вы сами ответили на свои вопросы:
>>>Но мне кажется разумнее было бы спросить – вдруг я не гордый и готов поучится ради достижения прозрачности архитектурных решений?:) Вместо этого вы просто решили все за меня и не оставили мне выбора
>>>Я и сам не очень то хотел идти (о чем я сразу написал)

Чтобы не было обид, скажу так: у вас код вовсе не плохой, но под наши высокие требования качества он не подходит.

Обычно мы отправляем исправления на доработки, когда замечания касаются исключительно особенностей строгого стиля кодирования. Но в вашем случае мы ждали нечто большее, особенно в некоторых спорных моментах. Например, дефайны конечно же применять возможно в C#, но от специалиста вашего уровня ожидалось более подходящее решение – вместо дефайна удачно подходило бы обобщение (языковое решение), или паттерн проектирования «стратегия» (ООП решение). То же самое касается и магических чисел – специалист высокого уровня просто не должен допускать подобные ошибки, с учетом что в тестовое задание очень простое и в нем очень мало кода.

Тестовое задание других кандидатов к сожалению вам отправить не могу, политика конфиденциальности не позволяет делать это.

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

ни фи га се переписочка 😎