Можете оценить уровень кода?

Question

[Дмитрий Башинский]

На днях писал тестовое задание, хотел бы чтоб оценило ещё пару лиц и дали оценку.
Расскажите чтоб улучшили или как бы сделали по другому?
Код клиента сгенерирован автоматически, его оценивать не нужно.

ТЗ
Решение

Comments

[sim3x]

Почему тз не в репозитории?

[l4m3r]

sim3x, а почему оно там должно быть?

[sim3x]

l4m3r, Потому что так проще и нам не нужно лазить по двум мертвым линкам

[Дмитрий Башинский]

sim3x, мертвые линки? Открываются ведь, я проверил.

[sim3x]

Дмитрий Башинский, а через месяц?

[Максим Федоров]

Мне все равно, но некоторым не понравится ваша пунктуация в описании

[Максим]

Дмитрий Башинский поддержу Максим Федоров , но не только про пунктуацию, а про стиль. Советую писать README в деловом стиле. Он нейтральный и никого не будет раздражать.

Answer 1

[ApeCoder]

Я бы попросил привести тесты к виду, в котором требования в ТЗ. Например как подписаться на rss фид после чего загрузить его содержимое. Соответствии проверить со айтемы попали в фид. Тесты просто проверяют на null возвращаемое значение.

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

Есть место исключение перехватывается и ничего с ним не делается

Comments

[Дмитрий Башинский]

В rss перевожу с помощью explicit, он расписан в модели.
Спасибо за ваш отзыв

[ApeCoder]

Дмитрий Башинский, только вы переводите, насколько я понял из rss, в свою модель, которую тоже называете RSS? Из-за этого мне код непонятен. То есть читателю кода надо догадаться чем отличается rss большими и маленькими буквами.

По Т.З. я вижу что требуется extensive caching.

Вот тут я не увидел кеширования собственно содержимого RSS.

public async Task<RssFeed> GetRssFeedAsync(int feedId)
        {
            var feed = await GetFeedAsync(feedId);

            var rss = rssReader.Read(feed.Url);

            return (RssFeed)rss;
        }

Теперь давайте подумаем, сколько мест надо поменять, чтобы научить это работать с еще одним форматом:
1) Надо добавить новый класс но аналогии с rss
2) надо добавить explicit конверсию из него в RSS
3) Надо вызвать эту конверсию

От расширяемого решения я бы ожидал дописывания в одном месте типа как плагина.

Answer 2

[⚡ Kotobotov ⚡]

объем выполненных работ впечатляет (желающих в никуда столько работать, не так много) -> соответственно как тестовое задание, безусловно вы справились успешно.
По поводу улучшений есть множество моментов - но они упираются примерно в одно - избыточность.
например очень большой размер некоторых классов, очень большое количество оберток, черезчур много проверок, много не нужных скобок, оберток и тд.
Вот например у вас множество переменных нулом инициализированы (возможно это такой стандарт в с$, я не сильно знаком)
При этом много где стоят геттеры без проверки на нул, в значения инициализированных нулом. Конечно во многих местах есть эта проверка, и это создает излишнюю избыточность в коде, возможно если бы вы сразу инициировали какими-то значениями по умолчанию (или сделали отложенную инициализацию до того как будет что-то известно про эти переменные), можно было бы избежать и ошибок, и лишних оберток.

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

П. С.
есть одна тонкость, в том что те кто делают тестовые задания - никогда их не проходят)
У нормальных разработчиков все эти типовые наброски уже в различных вариациях есть, они просто ссылаются на то что у них есть, нормальный разработчик не будет тратить на такое бесполезное занятие время, такое только джуны делают, а джуны как правило не нужны.

Comments

[Дмитрий Башинский]

Возможно вы говорите про код клиента?
Клиент сгенерирован автоматически, вся моя работа направлена на апи.

Если Ваш ответ был направлен на Client, то пересмотрите и оцените саму апишку, бизнес логику и доступ к данным (api, bl, da, core)
Спасибо за Ваш ответ:)

Answer 3

[basrach]

Пунктуация и стилистика бросается в глаза, вы можете аргументировать типа "не на писателя устраиваюсь же", но встречают по одежке, знаете ли...
По коду.
Вот вы говорите DDD, а похоже на Transaction Script. Поскольку в классе FeedLoader в каждом не-query методе происходит сохранение изменений, т.е. нет понятия бизнес транзакции. В данном случае это не особо нужно, но раз уж вы про DDD заикались... Также богатой изолированной модели не видно. Возможно в данном случае опять же эту богатую модель неоткуда взять, потому как предметная область тривиальная. Но тогда выходит, что вы выбрали неправильный способ решения задачи. В модуле Core у вас какие-то классы, очевидно для сериализации, непонятно что они делают в Core. Да и вообще, если бы увидел код без описания, мне бы никогда даже не закралась мысль, что его создавали по DDD.
API не REST, хотя в требованиях было про REST.
Нет конфигурирования кэша, что, где и насколько кэшировать. При этом само кэширование реализовано в BusinessLayer, что странно, поскольку кэш, как и, например, DataAccess, это чисто инфраструктурные вещи.
Логирование только в API и только ошибок.
В тестах нет моков. И похоже что тестирование только интеграционное, т.е. без базы данных не получится протестировать ничего.
Пустой catch в ApiTester. Да, это не основная кодовая база, но когда не работает и при этом не сообщает почему не работает, всегда бесит, даже если это тесты.
Итоговая оценка зависит от ваших ожиданий и ожиданий ревьюверов. Если вы junior, и на эту же то позицию претендовали, то вполне неплохо. Если senior, то зависит от уровня ревьюверов/проекта/компании и т.д.

Comments

[Дмитрий Башинский]

Спасибо за Ваш ответ. Я видимо не правильно перевел ТЗ, и подумал что наоборот, не нужно реализовывать REST