Прошу оцінити мій код

Вітаю, шановні. Я останні пару років самостійно вчився і тепер мені хотілося б почути думку профессійних розробників про мої роботи. Мені цікаво, наскільки погано чи дуже погано у мене виходить. А, може, навіть і не дуже то й погано. Я розумію, що витрачати час на г...код якогось чувака так-собі розвага, але якщо може хтось допомогти — дуже вдячний. Я не маю ні знайомих ані друзів програмістів, тож не маю можливості поспілкуватись та зрозуміти для себе свій рівень. Хоча б коротесенько, прошу. Велике дякую, мирного неба вам.

github.com/MaksimKasyanenko

👍ПодобаєтьсяСподобалось1
До обраногоВ обраному0
LinkedIn
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter

Коли я бачу список полів, які не розділені рядком, відразу розумію, що писав intern/junior.

private SelectedList(){}
public U Request{get;set;}
public List Items{get;set;}
public int PageQuantity{get;set;}

Я не intern і не junior. Я електромонтер і як раз намагаюсь зрозуміти, чи МІГ БИ я стати intern/junior. Звичайно, я мріяв стати спеціалістом в IT, та поки у мене була робота це було скоріше хоббі. Зараз, коли я вимушений починати життя з нуля, я маю обрати шлях... А ви просто мені у цьому допомагаєте ;-)

Я б ще додав:
1. Намагайтесь бити аппку на рівні (View, Domain, DataLayer).
2. Використання Session та ViewData/ViewBag краще звести до мінімуму (для зберігання корзину можна зберігати в браузері для неавторизованних користувачів та в базі для авторизованних, або обійтися лише базою)
3. Для валідаціі можна використати FluentValidator (справа смаку, але допомагая знемншити кількість кода та винести валідацію в окреме місце)
4. Раджу взяти за звичку писати кастомні Exception-и а не тільки кастомний меседж (так легше наприклад зробити ExceptionFilterAttribute/Middleware та слати 400/500/etc. в залежності від типу Exception-а (можливо там же й логувати))
5. В контроллерах не має бути логіки та View елементів (наприклад тексту для фронту)
6. Місцями моделі мають данні, що їм не притаманні. Наприклад PhoneNumber (номеру телефона не належить інформація щодо власника, радше навпаки)
7. Коли ви визначили що належить до Domain/DataLayer, можно розбити їх на меньші частини.
Для Domain гарно підійде концепція DDD (почитайте частину про BoundedContexts) + бажано відділяти контракти від імплементації (таким чином ви себе спонукаєте не використовувати деталі, лише контракти)
8. IMHO: я б ховав деталі EntityFramework на рівні DataLayer та використовував кастомні репозиторії (та інтерфейси для них) на рівні Domain (таким чином ви не тягаєте зайві dependencies та залишаєте собі якийсь простір для маневру в питанні вибору ORM).
9. В комментарях/error/validation message/etc. краще користуватися тільки англійською.
Інші мови маюсь залишатися на рівні Front-end або в resources для перекладів (але не факт що воно вам наразі потрібно)
10. Робота з датами: завжди порівнюйте дати в UTC (з явним приведенням). Це може здаватися чимось маленьким, але краще візьміть це за звичку одразу.

Взагалі, я б радше радив почитати про DDD, але обережно. Не все звідти потрібно брати для КОЖНОГО проекту, але воно гарно описує як треба підходити до бачення предментої області свого проекту (не знаю як виразити це доречніше).

Ще порадив би знайти когось, хто може приділяти вам декілька годин часу (на тиждень?) щоб зробити code review та порадити що почитати далі.

До CQRS ще порадив би подивитись event sourcing, але (як на мене) CQRS + event sourcing може бути не самою простою темою для розуміння (якщо ви початківець)

«8. IMHO: я б ховав деталі EntityFramework на рівні DataLayer та використовував кастомні репозиторії (та інтерфейси для них) на рівні Domain (таким чином ви не тягаєте зайві dependencies та залишаєте собі якийсь простір для маневру в питанні вибору ORM).»

Але ж тоді прийдеться писати багато одноманіиного коду, плюс втрачається потужність ORM фреймворку.

частково.
Так буде зайвий boilerplate.
Втрачається потужність ORM — не обов’язково.
Але саме через це, цей пункт це IMHO (ну тобто, це суб’єктивне як на мене)

По C# розказали. Розкажу про комміти. Вони мають відповідати певним крітеріям:
— автономність. Після кожного коміту проект має збиратись.
— атомарність. Не можна змішувати декілька логічно і функціонально різних змін в одному коміті.
— інкрементність. Коміти, що вводять нові сутності, мають бути раніше за код, який їх використовує.
У dfc є коміти, які міняють 10 речей одразу. Це робить підтримку проекта важкою.

Про пусті commit messages вже сказали. Тут непогано описаний загальний принцип написання commit messages:
initialcommit.com/...​t-messages-best-practices

Тож... до Junior мені ще дуже далеко, правильно?

Неправильно. Йдіть на інтерв’ю, фейліть, підучуйте що зафейлили і за роботу.

Один з сайтів явно комерційний, впиши в резюме як постійне місце роботи, бо технічно так і є

Дивлю тепер проект PlainStore

1. «—»

Такі коменти в комітах не робляться. Потрібно хоча б базово описати що робить коміт. Якщо це старт проекту з теймплейту то можно написати «Initial setup.»

2. Найменування папок проекту абсолютно дивне: Design, Domain, Web. Що таке Design? Теймлейт, чи? Далі Domain, ну може і ок як BusinessDomain, не скажу однозначно. Це якесь намагання створити three layer architecture?

Заходжу в Domain

3. В ItemSelector.cs по перше форматувнаня коду жахливе. Використовуйте якусь тулу аби до прикладу PerPage{get;set;} = 12; не збивалось в купу. Правильно: PerPage { get; set; } = 12;
3.1 ItemSelector іменування дженерік параметрів погане. Потрібно повне слово.
3.2 Декілька класів в одному файлі при чому змішаних абстрактні і звичайні. Абсолютно недопустимо.
3.3

protected int CountPageQuantity(int items)         {             int count = (items-items%PerPage)/PerPage;             if(items%PerPage > 0)count++;             return count;         }

Якась дивна формула. Почнемо з того що назва аргументу items не ідповідає його суті. Він тримає кількість айтемс а не самі айтемс. Перейменуйте на itemsCount.
Якщо мета функції визначити кількість сторінок то правильна формула ось: Math.Ceiling(itemsCount / PerPage).

3.4 BusinessLayer не має взаємодіяти з Request.
3.5 Увесь клас працює вже з витягнутими даними з бази. Не можна спочатку тягнути усю базу а потім на клієнті фільтрувати. По правильному робити через Expression<Func<TEntity, bool>>  які потім вкладаються в ef core AsQueryable функції типу Where.
3.6 Навіщо цей клас? SelectedList, чому йому передається U який ніде не використовується? По суті цілий зайвий клас все що робить це надає статичний метод Create для створення себе ж.з публічними пропертями які і так можна зафілити через new. Trash.

Ось моя капля в ревю. Пишіть якщо з чимось не згодні.

Загалом я б оцінив так: дуже багато коду, намагання в архітектуру, але навіть на рівні класу кваліті нема. Спочатку навчитися првильно писати прості програми зі стандартною архітектурою, а потім через роки промислового досвіду гратися в архітектора. Якщо кортить накласти якусь архітектуру, оберіть CQRS, там все просто і можна набити руку на правильний код. Не робіть ентерпрайз монстрів з додатків, тепер в моді простота.

Дуже дякую за вашу увагу, буду вчитись

Привіт. У .net не бум бум. Але що одразу бросається це скажімо дуже лаконічні коменти до комітів. За таке бьють по руках і дуже сильно.

him-test.somee.com — простий застосунок для тестування учнів

А можеш добавить скрінів і опису, що воно взагалі таке, що робить, для чого потрібно, і як його запустити?

Підписатись на коментарі