Wątek przeniesiony 2019-08-26 23:18 z przez cerrato.

Ocena projektu

Odpowiedz Nowy wątek
2019-08-26 20:32
0

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

edytowany 1x, ostatnio: Michał Warmuz, 2019-08-26 20:34

Pozostało 580 znaków

2019-08-26 20:41
1

https://github.com/Michal-War[...]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-War[...]ication/CommentService.cs#L40

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

https://github.com/Michal-War[...]ication/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-War[...]cation/CategoryService.cs#L31

To Entry to chyba standardowy sposób na aktualizację obiektu bazodanowego bez jego wcześniejszego pobierania z bazy (np. jak cały obiekt z id dostaniemy przez api). - some_ONE 2019-08-26 21:34
Nie pisałem, że to źle, po prostu się nie spotkałem - zwykle po prostu robiłem Attach - Pixello 2019-08-27 08:31

Pozostało 580 znaków

2019-08-26 21:52
1
  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.

edytowany 4x, ostatnio: rgawron, 2019-08-26 21:59
1) czemu akurat git flow? feature może robić na innym branchu, a merge jest akceptowany tylko jeżeli przechodzą testy, a to zapewnia jako taką stabilność. Wersje stabilne są releasowane. Po co w repo, w którym pisze się samemu żonglować sztucznymi branchami? - WeiXiao 2019-08-26 23:00
Masz rację. - rgawron 2019-08-26 23:09

Pozostało 580 znaków

2019-08-27 08:38

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-War[...]lers/CommentController.cs#L39

2) Sprawdzanie czy ModelState.IsValid powinno być w filtrze, patrz tu: https://benfoster.io/blog/aut[...]tate-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-War[...]lers/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-War[...]pplication/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/[...]ared/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.entityframeworktu[...]appings-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;)

edytowany 6x, ostatnio: nobody01, 2019-08-28 08:53

Pozostało 580 znaków

2019-08-27 08:43
0

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 :)

Pozostało 580 znaków

2019-08-27 09:47
2

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;
        }

Lubię miodek :)
A gdzie klamerki w 5 lini? :( - Terrored 2019-08-27 15:22
@Terrored: po co wydłużać sobie kod :P - WeiXiao 2019-08-29 01:36

Pozostało 580 znaków

2019-08-27 09:56
2

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[...]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 ;(

edytowany 7x, ostatnio: nobody01, 2019-08-27 10:42
Ależ i bez language-ext ten kod też będzie lepszy niż poprzednie propozycje. - somekind 2019-08-27 12:33

Pozostało 580 znaków

2019-08-27 12:33
4
nobody01 napisał(a):

I w ogóle dlaczego jest SingleOrDefault a nie FirstOrDefault?
https://github.com/Michal-War[...]pplication/PostService.cs#L81

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


"HUMAN BEINGS MAKE LIFE SO INTERESTING. DO YOU KNOW, THAT IN A UNIVERSE SO FULL OF WONDERS, THEY HAVE MANAGED TO INVENT BOREDOM."
Pokaż pozostałe 21 komentarzy
1) IMO znaczenie Single sobie dopowiadasz, bo przeczytałeś dokumentację: Single sugeruje jedynie, że zostanie zwrócony jeden obiekt, a nie, że innych nie ma. 2) Chcę też pobierać ze źródła poprawne obiekty. Też powinienem to zaznaczyć, odpalając walidator po pobraniu? - nobody01 2019-08-28 16:08
3) Pisząc o innych zastosowaniach LINQ, dałeś mi kolejny argument. Możemy przecież szukać po ogromnej liście znajdującej się w pamięci. Wtedy użycie SingleOrDefault powoduje oczywiste problemy wydajnościowe, jeśli lista nie jest posortowana. No chyba, że założymy, że szukamy po bazie mającej indeksy, ale to oznacza wyciek abstrakcji. - nobody01 2019-08-28 16:16
Chcę otrzymać jeden obiekt spełniający warunek wyszukiwania. To musi być jeden obiekt, nawet jeśli w źródle danych (nieważne, czy bazie, czy pamięci) jest ich omyłkowo tysiąc. Czego mam zatem użyć? - somekind 2019-08-28 17:01
IMO zadne rozwiazanie nie jest dobre. Edytowalem juz wczesniej post. Ja teraz jedynie bronie uzywania FirstOrDefault jako alternatywy dla SingleOrDefault w wielu przypadkach. To chyba konczy dyskusje, bo mozna by sie klocic w nieskonczonosc :) - nobody01 2019-08-28 17:36
zadne rozwiazanie nie jest dobre - no na pewno biznes i klienci zaakceptują taki argument. :P - somekind 2019-08-29 00:57

Pozostało 580 znaków

2019-09-03 17:51
0

https://github.com/Michal-War[...]master/Blog.Model/Category.cs

Nazwa Katwgorii? :D


A nie tak się piszę :D Sam już nie wiem xd - Michał Warmuz 2019-09-04 11:25
Fuck sorrki nawet jak mi napisałeś nie zauważyłem błędu :D Juz poprawiam xd - Michał Warmuz 2019-09-04 11:26

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0