Ocena projektu

Cześć. Ostatnio napisaliście mi wiele uwag bardzo słusznych bardzo dziękuje :) Wprowadziłem poprawki i napisałem bloga prostą aplikację. Mam prośbę napisalibyście mi znowu co mam poprawić co usunąć co dodać. Oraz taką ocene od 1 do 10 co sądzicie o tej aplikacji. Z góry bardzo wam dziękuje za pomoc. I sądzicie czy mógłbym już starować na juniora ?
https://github.com/Michal-Warmuz/Blog-v.1.0

https://github.com/Michal-Warmuz/Blog-v.1.0/blob/master/Blog.Model/Category.cs

Te errory w atybutach, nie patrzyłem szczegółowo, ale to trochę antywzorzec, poczytaj o i18n

https://github.com/Michal-Warmuz/Blog-v.1.0/blob/master/Blog.Application/CommentService.cs#L40

Ciekawe zastosowanie Entry(x), skąd to wziąłeś?

https://github.com/Michal-Warmuz/Blog-v.1.0/blob/master/Blog.Application/CommentService.cs#L40 Takie rzeczy to raczej powinieneś mieć gdzieś na froncie, co jeżeli chcesz wyświetlić tylko kilka pierwszych wyrazów na telefonie, a paragraf na tablecie?

Ogólnie to na pierwszy rzut oka jakoś nie mogę się przyczepić za bardzo, a bym chciał - testy prawie nie istnieją.

To się robi kaskadowym usuwaniem, poczytaj sb
https://github.com/Michal-Warmuz/Blog-v.1.0/blob/master/Blog.Application/CategoryService.cs#L31

1. Wszytko wrzucasz na master'a przydałby się develop i feature/bugfix branche - zobacz git flow.
2. W nazwie repo masz rewizję, jeśli ją zmienisz, będziesz musiał zmieniać nazwę swojego repo.
3. Przydałoby się CI. Do CI możesz za darmo używać Travisa.
4. README.md niewiele mówi komuś kto chciałby ściągnąć to repo i ustawić sobie środowisko, by z nim pracować, albo po prostu wiedzieć, co to za projekt.

1. ViewModele i DTO powinno się tworzyć nie tylko dla wyjścia, ale także dla wejścia, także powinieneś mieć raczej jakieś CommentPostDto, które przekazujesz do kontrolera, a następnie pchasz do CommentService.
   https://github.com/Michal-Warmuz/Blog-v.1.0/blob/c035347c9056150bcff8d5cda92c7cda83a2e7b8/Blog.WebUI/Controllers/CommentController.cs#L39

2. Sprawdzanie czy ModelState.IsValid powinno być w filtrze, patrz tu: https://benfoster.io/blog/automatic-modelstate-validation-in-aspnet-mvc

3. IMO zamiast przekazywać do serwisów Id użytkownika z kontrolera lepiej stworzyć sobie jakieś w Contracts jakiś IUserContext, zaimplementować go sobie w WebUI i wstrzykiwać przez DI, gdy zajdzie potrzeba.

4. Po co taki log? https://github.com/Michal-Warmuz/Blog-v.1.0/blob/c035347c9056150bcff8d5cda92c7cda83a2e7b8/Blog.WebUI/Controllers/CommentController.cs#L54 Logów raczej używa się w przypadku błędów, tzn. leci sobie wyjątek, przechwytujemy go w filtrze, logujemy, co się wywaliło, i zwracamy 500.

5. W serwisach aplikacyjnych zakładasz happy path. Jeśli użytkownik będzie chciał zobaczyć post, który został usunięty, to dostanie informację o błędzie serwera zamiast o tym, że posta nie ma, bo poleci NullReferenceException. I w ogóle dlaczego jest SingleOrDefault a nie FirstOrDefault? (EDIT: Jeśli SingleOrDefault jest dla Ciebie czytelniejsze, to go używaj.)
   https://github.com/Michal-Warmuz/Blog-v.1.0/blob/c035347c9056150bcff8d5cda92c7cda83a2e7b8/Blog.Application/PostService.cs#L81

6. Problem z poprzedniego punktu możesz rozwiązać, zwracając z serwisów Result, taki jak tu: https://github.com/CoyoteNET/CoyoteNET-backend/blob/master/Coyote/CoyoteNETCore.Shared/ResultHandling/Result.cs Do tego możesz utworzyć sobie jakiś kontroler bazowy, w którym będziesz miał metodę mapującą Result na ActionResult, np. var result = await myService.DoSth(); return CreateResponse(result, () => Json(result.Value)) (ta lambda to takie onSuccess, tzn. wszystko poszło pomyślnie, zwracamy JSONa)

7. Wstrzykiwanie CategoryService, CommentService i UserService do PostService to przykład tworzenia powiązań między klasami tej samej warstwy. To raczej nie prowadzi do niczego dobrego (łatwo dostaniesz jakąś cykliczną referencję). Zamiast CommentService możesz po prostu zrobić cascade delete, zamiast UserService możesz dać IUserContext i zamiast CategoryService po prostu możesz użyć ORMa.

8. Dalej wszystko jest w Service. Przeczytaj jeszcze raz swój ostatni wątek z recenzją kodu. ;)

9. Po co te interfejsy do serwisów? Interfejsy raczej stosuje się do rzeczy infrasrukturalnych, np. IUserRepository, IEmailSender, IUserContext, żeby można było kod korzystający z nich łatwo i szybko przetestować jednostkowo albo móc sobie podstawić jakąś testową implementację w testach "integracyjnych".

10. Powinieneś sobie oddzielić konfigurację modeli bazodanowych od nich samych, bo to narusza SRP i zmniejsza czytelność. Poczytaj o Fluent API: https://www.entityframeworktutorial.net/code-first/configure-property-mappings-using-fluent-api.aspx

I nie przejmuj się, że ludzie ciągle Ci tyle błędów wytykają. Dzięki tym paru postom osiągniesz taki poziom, na który kiedyś ludzie pracowali kilka lat;)

Jasne nie przejmuje się wrecz przeciwnie właśnie bardzo jestem wdzięczny że komuś się chce sprawdzić mój kod i napisać co robie źle, Bardzo dziękuje. Napiszę kolejny projekt i wszystko poprawię co napisaliście Dziękuje :)

Pamiętaj o refaktoryzacji swojego kodu. Przykład. Zamiast takiego tworka możesz to krócej zapisać
Twoja wersja

public string GetShortContent(string content)
        {
            string result = "";
            if(content == null)
            {
                return result;
            }
            else
            {
                if (content.Length > 40)
                {
                    result = content.Substring(0, 40);
                    result = result + "...";
                    return result;
                }
                else if (content.Length <= 40)
                {
                    result = content;
                }
                return result;
            }
        }

Po skróceniu:

        public string GetShortContent(string content)
        {
            string result = "";
            if (string.IsNullOrEmpty(content))
                return result;
            if (content.Length > 40)
            {
                result = content.Substring(0, 40);
                result += "...";
                return result;
            }
            return content;
        }

Albo jeszcze krócej ;)

public string GetShortContent(string content)
        {
            // Walidacja w warstwie aplikacji zawiodła, więc mamy sytuację wyjątkową i pozostaje jedynie rzucić wyjątek
            if (string.IsNullOrWhiteSpace(content)) 
                throw new ArgumentException("Content cannot be null or white space.");
            
            return content.Length <= 40 ? content : $"{content.Substring(0, 40)}...";
        }

Można też zwrócić Option. Tu fajny artykuł o tym, jak obsługiwać nulle w C# przy użyciu language-ext: https://templecoding.com/blog/2017/01/31/handling-nulls-in-csharp-the-right-way Miałbyś wtedy:

public Option<string> GetShortContent(string content)
        {
            if (string.IsNullOrWhiteSpace(content)) return None;
            return Some(content.Length <= 40 ? content : $"{content.Substring(0, 40)}...");
        }

Ale to raczej ciekawostka. Pewnie nikt nie używa language-ext w Polsce ;(

nobody01 napisał(a):

I w ogóle dlaczego jest SingleOrDefault a nie FirstOrDefault?
https://github.com/Michal-Warmuz/Blog-v.1.0/blob/c035347c9056150bcff8d5cda92c7cda83a2e7b8/Blog.Application/PostService.cs#L81

A czemu miałoby być FirstOrDefault? Spodziewasz się kilku postów o tym samym Id?

https://github.com/Michal-Warmuz/Blog-v.1.0/blob/master/Blog.Model/Category.cs

Nazwa Katwgorii? :D