Wątek przeniesiony 2017-06-26 00:44 z C# i .NET przez somekind.

Prośba o ocenę kodu

0

Witam,
staram się o posadę młodszego programisty .net. Stworzyłem aplikację kadrowo-pracowniczą w asp.net mvc (wiadomo html,css, troche javascriptu,ajaxa).
Prosiłbym o ocenę czy z takim kodem bez wykształcenia informatycznego mam szansę na pracę jako młodszy programista. Z góry dziękuję za odpowiedź.
https://github.com/kamilkieszek/AKPRepository
Pozdrawiam
Kamil

0

Na pierwszy rzut oka to projekt nie jest podzielony na warstwy i w controllerach się dzieje dużo za dużo. Między controllerem a bazą danych warto mieć warstwę logiki biznesowej.

0

Dlaczego nazwy niektórych klas poprzedzasz literką I? :P

Jak używasz viewmodeli to po co w modelu bazodanowym dajesz atrybuty związane z widokiem? (HiddenInput)

1
  1. Nie wrzucaj packages do Gita, przy kompilacji VS pobierze Nugetem wszystkie potrzebne paczki.
  2. Nie wrzucaj .vs do Gita, nikogo nie obchodzą Twoje ustawienia.
  3. Nie wrzucaj bin, obj i innych artefaktów kompilacji do Gita.
  4. Nazwy zmiennych zaczynamy małymi literami.
  5. Z kolei właściwości zaś dużymi.
  6. Czemu interfejsy do repozytoriów masz w Infrastructure? To raczej logika biznesowa.
  7. Czemu w ogóle masz interfejsy, skoro nie masz warstw, testów ani IoC?
  8. Czemu wszystkie repozytoria masz jako pola klasy UnitOfWork? To jest niezgodne chyba ze wszystkimi możliwymi zasadami SOLID.
  9. Nie mieszaj języków jak tutaj:
public enum PeriotList
 {
            Miesiąc,
            Kwartał,
            Rok
}
0

Dziękuję za ocenę, jednak mam jeszcze pytania. 6. W takim razie gdzie dla przykładu powinienem umiesci logikę biznesowa? 7. Co masz na myśli pisząc "skoro nie masz warstw"? A model widok kontroler? Czy nie jest to wystarczające? 8. Możesz podać przykład rozwiązania problemu? (bede wdzieczny za link z dobrym przykladem) No i pytanie czy myślisz, że z takim kodem w ogóle mam szanse na pierwszy staż, prace?

1

Logika biznesowa powinna być najlepiej w osobnym projekcie. W dużym uproszczeniu twój solution mógłby składać się z 3 projektów: logika dostępu do danych, logika biznesowa oraz właściwa aplikacja która tamtą logikę będzie konsumować.

Jak powinno to wyglądać, żeby było w miarę sensownie (też w dużym uproszczeniu)? W projekcie z dostępem do danych trzymasz klasy odpowiedzialne za komunikację z bazą, żadnej innej logiki, żadnych walidacji, zwykłe wstawianie, pobieranie etc. Logika biznesowa będzie odpowiadać za.. logikę biznesową. Czyli na przykład walidację tego co użytkownik wprowadził, czy nie próbuje wykonać operacji zabronionej dla niego. Cały silnik aplikacji to będzie twoja logika biznesowa. Powinna być napisana tak, że w każdej chwili zakładasz nowy projekt, dla przykładu w WPF'ie i jesteś w stanie szybko napisać taką sama aplikację bez edycji istniejących klas. Natomiast sam konsument (w twoim przypadku MVC, ale może być tez wspomniany WPF) jedyne co robi i korzysta z logiki biznesowej i wykonuje kod konkretny dla technologii (np. dla MVC coś związanego z ViewBag, które jest ohydne, lub data binding dla WPF'a). W przypadku MVC mogłoby to wyglądać tak, że jedyny kod w controllerach to odpalenie serwisu z logiki biznesowej i zwrócenie odpowiedniego widoku. Dodatkowo serwisy mogłyby być wstrzykiwane przez kontener DI do controllerów.

Opisałem to w bardzo dużym uproszczeniu więc możliwe, że będzie hejt ze strony mądrzejszych, ale mam nadzieje, że pomoże ci to zrozumieć na czym polega separacja odpowiedzialności w aplikacji.

Samo używanie frameworka z MVC w nazwie nie spowoduje, że projekt będzie wykorzystywać MVC w odpowiedni sposób.

1
kieszinho10 napisał(a):
  1. W takim razie gdzie dla przykładu powinienem umiesci logikę biznesowa?

Na początek będzie dobrze, jeśli oddzielisz ją od kontrolerów i DAO, i umieścisz w jakichś klasach serwisów w oddzielnym katalogu. W następnym kroku dobrze byłoby podzielić solucję na projekty.

  1. Co masz na myśli pisząc "skoro nie masz warstw"? A model widok kontroler? Czy nie jest to wystarczające?

MVC to wzorzec warstwy prezentacji, sam z siebie nie mówi nic o warstwach: http://commitandrun.pl/2016/05/30/Brutalne_prawdy_o_MVC/

  1. Możesz podać przykład rozwiązania problemu? (bede wdzieczny za link z dobrym przykladem)

Najlepsze rozwiązanie to pozbycie się repozytoriów. Ale jeśli chcesz je mieć, to repozytoria powinny być zależne (czyli przyjmować w konstruktorze) obiekt odpowiedzialny za realizację unit of work. Dodając nowe repozytorium nie powinieneś dotykać klasy implementującej UoW.

No i pytanie czy myślisz, że z takim kodem w ogóle mam szanse na pierwszy staż, prace?

Może na staż... Nie zatrudniam ludzi, więc ciężko mi odpowiadać na takie pytania.

0

w CSS stylujesz za pomocą id a nie klas:

#Container {...}
#AP {...}
#Work {...}

i tak dalej. Rzadko kiedy jest to potrzebne, a tylko rzucasz sobie kłody pod nogi (bo element z danym id może być tylko jeden. Więc jeśli będziesz chciał zrobić np. dwa kontenery albo dwa elementy work - a zwykle wcześniej czy później tak będzie - to będziesz musiał przerobić id na klasy CSS, i selektory z #container na .container (które to selektory trochę inaczej się stylują - dla przeglądarki selektor id jest mocniejszy od selektora klasy).

Więc lepiej się nie bawić w id i od razu robić na klasach CSS (id jako takie mają czasami zastosowanie, ale prędzej np. do identyfikacji elementów w JS, jak faktycznie wiesz, że jest jeden element na stronie i chcesz się do niego dostać. Albo do robienia kotwic. Do CSS to nie wiem czy w ogóle jest sens używania id w selektorach)

No ale na tych nieszczęsnych id mógłbyś pewnie pojechać z parę miesięcy, zanim byś odczuł ból. Ale np. tu masz selektory, które ci się rozlecą jutro albo pojutrze:
ol > li > ul > li > a
ol > li > ul > li > a:hover

Takie sztywne zakładanie struktury HTML jest bardzo nierozsądne, bo dodasz jeden poziom więcej czy odejmiesz (co może zdarzyć się w dowolnym momencie) i ci się rozleci.

albo gdzie indziej też masz podobne selektory:

.pagination > .active > span:hover,

jeśli zamienisz span na cokolwiek innego (na div choćby) to też się rozleci z hukiem. Nie mówiąc już o tym, że takie selektory są nieczytelne. Lepiej już dodać kolejną klasę do spana w samym środku i selektować po tej klasie...

0

@somekind, a czy nie masz może jakiegoś linka z prostym przykładem jak to powinno wyglądać? Na ten artykuł trafiłem już wcześniej. Ale wszelkiego rodzaju tutoriale i kursy dotyczące ASP MVC są zaprzeczeniem tego artykułu. Ja też dopiero zaczynami i póki co , to takie coś utkwiło mi w głowie. Tylko proszę nie podawaj mi linka dla git'a tylko coś w stylu step by step

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