Ответы пользователя по тегу Code review
  • Можно код-ревью проекта на C# с юнит тестами?

    vabka
    @vabka Куратор тега C#
    Токсичный шарпист
    Сначала замечания по реализации:

    1. Раз уж мы делаем свой аналог List - тогда есть смысл вынести его в библиотеку, а не с Exe.
    TargetFramework стоит делать чуть более новым. Сейчас LTS - .NET 6 (но это не так уж и критично)

    2. Почему List Только для char реализован? Почему не сделать его обобщённым?

    3. Почему List не реализовывает стандартные интерфейсы? Тот же IEnumerable<T> хотябы.

    4. PrintForward - явно какой-то лишний метод. Список не должен отвечать за вывод в консоль.

    5. Публичный сеттер в Node выглядит как что-то опасное. Так можно изменить Next или Previous - лист изменится, вплоть до изменения количества элементов, но значение Count в самом листе не изменится, от чего всё поломается.

    А вот тесты наоборот выглядят вполне неплохо, но надо бы покрытие посмотреть.
    Ответ написан
    3 комментария
  • Ревью кода. Консольное приложение для создания и прохождения викторин. C#?

    vabka
    @vabka Куратор тега C#
    Токсичный шарпист
    1. Не ясно зачем использовал старый .net framework
    2. Зачем у Score публичные геттеры-сеттеры, если по идее класс иммутабельный и у него даже конструктор с обязательными параметрами есть?
    3. Везде есть привязка к русскому языку, что не очень хорошо, но и не смертельно.
    4. Есть небольшая проблема с именованием методов, но не критично
    5. Стандартная бинарная сериализация - зло почти всегда.
    6. Пароли в открытом виде, и даже нет никакой абстракции над форматом хранения данных (если бы была абстракция, то уже не так критично было бы)
    7. Относительные пути (нашёл в Manager.cs). Сами по себе относительные пути - это не плохо, но тут аж в родительскую папку стучится.
    8. Не понятно, почему есть всего три захардкоженных вида викторин. Разницу между ними по коду я не увидел.
    9. Главное меню в процедурном стиле с совершенно неговорящими именами методов и константами - ну это совсем уже ни в какие ворота.
    10. Нравится, что сразу есть разделение на два bounded context - прохождение викторин и создание. В принципе не сложно будет зарефакторить.

    Дальше не буду проверять. Кажется, что автор просто не достаточно проработал предметную область и не проработал архитектуру приложения, на основе этого.

    Советую почитать книги:
    "Чистая архитектура" Роберта Мартина
    "Предметно-ориентированное проектирование (DDD). Структуризация сложных программных систем" Эрика Эванса

    Не обязательно прямо целиком книги - можно просто почитать статьи и посмотреть доклады про то как следует строить архитектуру приложений.
    Вкратце:
    1. Старайся не смешивать модель предметной области и детали реализации
    2. Если делаешь обогащённые модели (с методами, которые отражают суть предметной области), то используй их повсеместно.
    3. Если делаешь анемичные модели - делай их повсеместно. Не стоит в рамках одного контекста смешивать оба подхода
    4. Код в идеале должен выглядеть так, чтобы было сразу понятно, что происходит и почему. Также должно быть очевидно, где и что нужно искать.
    5. В идеальном мире, код должен быть такой, что если бы тебе дали задачу "перенести это всё в веб", тебе бы вообще не пришлось никак менять код ядра (в данном случае проект QuizModel), и при этом не пришлось бы дублировать код из QuizCreator и QuizApp
    Ответ написан
    1 комментарий
  • На каком я сейчас уровне?

    vabka
    @vabka Куратор тега C#
    Токсичный шарпист
    1. Не умеешь пользоваться гитом, тк загрузил файлы через Upload
    2. Код невозможно проверить, тк ты закинул только cs файлы, но не приложил не менее важный csproj - не разбираешься, как собирается проект.
    3. Про свич кейс тебе уже сказали в комменте.
    Про остальное мало что можно сказать - код самый обычный, на три с минусом, не очень хороший, но и не слишком ужасный.
    С архитектурой плохо, тк детали реализации смешаны с бизнес-правилами.
    По алгоритмам нельзя оценить, тк никаких сложных алгоритмов в твоём проекте нет.

    Уровень, имхо: стажёр/младший разработчик(обязательно под менторством и наблюдением более опытного)
    Ответ написан
    5 комментариев
  • Нормально ли разбивать все на такое количество методов?

    vabka
    @vabka Куратор тега C#
    Токсичный шарпист
    выглядит хорошо, за исключением неиспользуемых методов.
    Лучше так, чем нагромождение кода.

    Один мне говорит что нужно чтобы методов было мало, а другой что много.

    А почему тогда у этих же людей не спросили, почему они так говорят?
    Ответ написан
    8 комментариев
  • Что выбрать: наглядность или краткость логических условий?

    vabka
    @vabka
    Токсичный шарпист
    Явное лучше, чем неявное.
    Код чаще читается, чем пишется.
    Так что лучше второй вариант.
    Ответ написан
    3 комментария
  • Как сделать из этого метода нормальный код?

    vabka
    @vabka Куратор тега C#
    Токсичный шарпист
    Код в целом норм, если не упарываться в Linq.
    Использовать linq не советую, тк он заметно медленнее и потребляет больше памяти, чем циклы.

    Если рефакторить, то целиком, включая GridRepository.
    Например можно зная текущую клетку и оффсет найти за O(1) все нужные клетки и отбросить занятые.
    Ответ написан