Czesc chlopaki.
Mam problem troche - nie ogarniam logiki wedle ktorej moj TL pisze kod przez co jak dostaje od niego review to mam srogie WTF.
Mam wrazenie, ze zawsze z najmniejszej glupoty, jak np jakis prosty check bo bazy w EF Core chlop tak mocno komplikuje sprawy, ze az mam z nim zwarcia.
Czy w takich sytuacjach odpuszczacie i potakujecie jak baranki czy gryziecie do krwi, bo do chlopa nie docieraja pewne argumenty...?
Chetnie poczytam Wasze opinie,
Serdecznie pozdrawiam.
Daj przykład tego skomplikowanego kodu. Gadanie o czyimś kodzie bez możliwości zapoznania się z nim jest słabe.
Ok.
Przyklad prosty - chcemy sprawdzic, czy w momencie dodania/edycji encji nie ma czasami takiej o takiej samej nazwie juz w bazie. Ja to zrobilem w query servicie dodajac metode IsEntityNameInUse i przerzucilem to na baze zeby sobie EFCore ogarnal.
Po review wyszlo cos takiego tyle ze juz w serwisie realizujacym logike:
private async Task<bool> IsEntityNameInUse(string entityName, int? entityIdToExclude = null)
{
var entities = await _entityQueryService.GetAllEntitiesAsync(EntityStatus.Active, entityName);
if (entityIdToExclude != null)
{
entities = entities.Where(d => d.Id != entityIdToExclude).ToList();
}
return entities.Any();
}
I mam z tym kodem co najmniej dwa problemy:
Jako szeregowy klepacz mam wyj*** z reguły. Szkoda czasu na kopanie się z koniem i jest zarąbiście. Dostaje hajs za godzinę a nie % z sprzedanego produktu.
Tylko są dwie rzeczy do zapamiętania w tym podejściu:
Po pierwsze - a to nie jest jakiś C# framework że pod spodem generuje dobrego SQLa? (pytam bo C# nie znam)
Po drugie - ja to bym unique na bazie założył, bo takie sprawdzanie w środowisku wielowątkowym może nie zadziałać
Takie rzeczy powinien ogarnąć DbContext tak jak piszesz.
To z czym ja mam problem to po co wam ta bezsensowna dodatkowa warstwa abstrakcji QueryService? Jaką wartość biznesową wam wnosi? W tej sytuacji powinniście w serwisie aplikacyjnym operować na DbContextcie. Robicie sztukę dla sztuki chcąc ukryć szczegół implementacyjny jakim jest ORM i komplikując całe rozwiązanie wprowadzając jakieś dziwy w postaci query/write serwisów.
Żeby rozdzielić odczyt od zapisu. No ale to prawda jest, że mega to jest lipne jak masz serwis który ma logikę a wewnątrz zależność na repo i query service. Nie wiem po kiego grzyba...
Żeby rozdzielić odczyt od zapisu.
To nie jest odpowiedź na moje pytanie tego można się domyślić. Ale jeśli nie ty jesteś autorem tego rozwiązania to zwalniam Cię z odpowiedzi :D
Poza tym w takiej sytuacji do projektu się bierze Mediatr i robi bieda CQRS-a. Ale nawet wtedy command/query/event handlery robią za serwis aplikacyjny i operują bezpośrednio na DbContexcie czy innym ISession z ORMa.
Generalnie jak tak wygląda architektura całej aplikacji to jest do całkowitego przepisania, zwłaszcza warstwa serwisowa
@markone_dev: generalnie to jeden z projektów typu one man army chłop napisał a Wy rozbudowujcie 
Mam wrażenie, że to strasznie syfiasta architektura.
W każdym razie
a)Zapinasz unique na tej nazwie i elo a nie w jakieś walidacje się bawisz.
b)tak jak piszesz jak już to trzeba zrobić to można zrobić opcjonalny argument w postaci delegaty. a nie jakieś ifologie i dodatkowe operacje na kolekcji i potem cast na liste.
Odpowiadając na pytanie - niestety tak jest, że niektórzy nie wiedzą po co i do czego code review jest; i traktują to jako swoje personalne narzędzie do narzucania innym swoich preferencji. Być może Twój TL właśnie to robi.
markone_dev napisał(a):
To z czym ja mam problem to po co wam ta bezsensowna dodatkowa warstwa abstrakcji
QueryService? Jaką wartość biznesową wam wnosi? W tej sytuacji powinniście w serwisie aplikacyjnym operować na DbContextcie. Robicie sztukę dla sztuki chcąc ukryć szczegół implementacyjny jakim jest ORM i komplikując całe rozwiązanie wprowadzając jakieś dziwy w postaci query/write serwisów.
To co robią to akurat odseparowanie logiki aplikacji od persystencji, i moim zdaniem to akurat dobre podejście - pod warunkiem że jest to wykonane odpowiednio.
PS: Chociaż tak jak to czytam, to jak robisz runtime check na takim Where, to to jest wykonane źle. Ten entityIdToExclude powinien wejść do tego query service.
moim zdaniem to akurat dobre podejście - pod warunkiem że jest to wykonane odpowiednio.
Nie jest. Co ci z tej dodatkowej warstwy abstrakcji? Jak będziesz chciał zmienić EF Core na NHibernate czy jakiegoś Dappera to i tak masz do przebudowy cały model domenowy wiec od groma roboty. Jak ktos mysli ze zmiana orm-a w enterprajsowej aplikacji to napisanie nowych configow dla encji i przepiecie implementacji w kontenerze DI to gratuluje poczucia humoru. Przepisywalem tak z NHibernate na Dapper i dodatkowa warstwa abstrakcji nic nie dala bo filozofia uzywania ORMa jest inna w kazdym ORMie.
DbContext czy ISession z Nhibernate ma wszystko co potrzeba. Transakcyjnosc, unit of work, tracking changes, optimistic locking i wiele wiele innych.
Jeżeli op nie robi ddd to nie ma sensu wprowadzać dodatkowej warstwy w postaci serwisów persystencji czy repozytoriów.
markone_dev napisał(a):
Nie jest. Co ci z tej dodatkowej warstwy abstrakcji? Jak będziesz chciał zmienić EF Core na NHibernate czy jakiegoś Dappera to i tak masz do przebudowy cały model domenowy wiec od groma roboty.
Jeśli masz to odpowiednio zrobione to nie. Wtedy to jest względnie łatwe.
markone_dev napisał(a):
Jak ktos mysli ze zmiana orm-a w enterprajsowej aplikacji to napisanie nowych configow dla encji i przepiecie implementacji w kontenerze DI to gratuluje poczucia humoru. Przepisywalem tak z NHibernate na Dapper i dodatkowa warstwa abstrakcji nic nie dala bo filozofia uzywania ORMa jest inna w kazdym ORMie.
DbContext czy ISession z Nhibernate ma wszystko co potrzeba. Transakcyjnosc, unit of work, tracking changes, optimistic locking i wiele wiele innych.
To znak, że nie było to zrobione dobrze.
markone_dev napisał(a):
Jeżeli op nie robi ddd to nie ma sensu wprowadzać dodatkowej warstwy w postaci serwisów persystencji czy repozytoriów.
No tak.
Widzę co próbowali zrobić, ale nie udało im się to.
rjakubowski napisał(a):
- jaki jest sens strzelania po wszystkie encje i wybierac z tego jedna, jesli typ zwracany to lista i musimy robic dodatkowy typecast?
Bonus - po co ta ostatnia operacja jest robiona w pamieci...? (Where statement w ifie)
A to serio najpierw wyciąga wszystkie rekordy z bazy i w pamięci sprawdza?
Widząc takie abstrakcje nad DbContextem to w pierwszej chwili jednak się spodziewałem, że GetAllEntities zwraca IQueryable i materializacja zapytania jest dopiero w .ToList() albo .Any() :D
Tzn. może trochę go kumam że chce ten QueryService utrzymać prosty i nie dodawać do niego metod na wszystkie możliwe okazje tylko obsłużyć to w miarę możliwości w serwisie trzymającym logikę - to się nawet trochę trzyma kupy tylko to obejście problemu który stworzył sam QueryService i który chyba nie jest potrzebny. Też ta operacja którą robi w pamięci nie jest kosztowna bo pobierasz jakiś rekord i zakładam że będzie tylko 1 albo 2 wyniki z czego chcesz jeden warunkowo wykluczyć. Ale nawet zakładając że jego kod jest w porządku to można go skrócić do formy
private async Task<bool> IsEntityNameInUse(string entityName, int? entityIdToExclude = null)
{
var entities = await _entityQueryService.GetAllEntitiesAsync(EntityStatus.Active, entityName);
return entities.Any(d => d.Id != entityIdToExclude);
}
dodatkowy plus że nie mamy materializacji .ToList na wszystkich wynikach tylko kończymy na pierwszym.
Nie rozumiem tylko po co w ogóle jest to entityIdToExclude, skoro chcesz sprawdzić czy entity o tej nazwie istnieje, ale chcesz jednocześnie dopuścić przypadki gdzie duplikaty są dozwolone?
No nie do końca wszystkie bo tam jest queryable, ale dla mnie to jest z d*py za przeproszeniem. Wolałbym mieć metodę na query service która robi checka i nara - czysto, jasno, precyzyjnie i wiadomo o co kaman, ale wg ziomka to overhead 
za to to podejście nie jest w ogóle overhead
Przy update trzeba wziąć pod uwage, że jeśli zmienisz inne pole a nazwa zostanie ta sama to chcesz wykluczyć obecnie nadpisywana encje. Mam już tego tematu dość na dziś
to nie wystarczyłoby sprawdzać to tylko w przypadku zmiany nazwy? Ostateczne sprawdzenie i tak powinno lecieć przy zapisie z constrainta na bazie
To znak, że nie było to zrobione dobrze.
Takim argumentem to można wszystko zanegować.
Po prostu model domenowy dla EF Core ma inne wymagania niż model dla Dappera czy NHibernate'a więc dla rzeczy bedzie inaczej wyglądał. Jedynym rozwiązaniem jest posiadanie dwóch modeli domenowego i persystencji ale nikt tak nie robi.
markone_dev napisał(a):
To znak, że nie było to zrobione dobrze.
Takim argumentem to można wszystko zanegować.
Po prostu model domenowy dla EF Core ma inne wymagania niż model dla Dappera czy NHibernate'a więc dla rzeczy bedzie inaczej wyglądał. Jedynym rozwiązaniem jest posiadanie dwóch modeli domenowego i persystencji ale nikt tak nie robi.
No i to jest słabe rozwiązanie. EF Core to nie jest coś co ma model domenowy (Tzn. możesz sobie tak to nazywać, jesli chcesz), ale to nie jest nic innego jak tylko abstrakcja na bazę danych. Zaawansowana, to prawda, ma dużo fajnych feature'ów, ale to jest abstrakcja na bazę danych - i jako taka powinna być oddzielona od logiki biznesowej. Jeśli tego nie zrobisz, to po prostu masz połączoną logikę z EF Core i tyle. Niektórym to pasuje. Ale jeśli je oddzielisz od siebie (i oddzielisz je odpowiednio), to jej podmianka na inną jest jaknajbardziej możliwa. Jeśli ktoś starał się je oddzielić od siebie, ale w taki sposób że nadal jego podmiana nie jest możliwa, to siłą rzeczy zrobił to nie udolnie.
Pracowałem nad projektami gdzie taki EF Core był całkowicie oddzielony od logiki aplikacji, i można go było podmieniać do woli. Rozumiem że większość forumowiczów mogła nigdy nie pracować nad takim projektem, no ale one istnieją.
@Riddle: Wszystko da sie latwo podmienic, zrobic i w ogole to nigdy nie ma zadnych problemow. Po prostu trzeba to zrobic dobrze. Problem polega na tym, ze w wiekszosci firm rozwiazania informatyczne sa tylko srodkiem do celu i nie zawsze ma sie nieograniczone zasoby jak:
a) czas
b) pieniadze
Jak tak czasem czytam Twoje wypociny w tematach z dzialu kariera / inzynieria oprogramowania to mam wrazenie, ze jestes jakims ewenementem na skale Polski albo nawet i calego swiata. Jedyny sluszny programista, ktory robi wszystko zgodnie z fachem, nie przeciaga terminow, zespol IT go czci, a biznes jest w nim po uszy zakochany. A to wszystko za sprawa jednego, prostego tricku tzn wystarczy robic dobrze.
Seken napisał(a):
@Riddle: Wszystko da sie latwo podmienic, zrobic i w ogole to nigdy nie ma zadnych problemow. Po prostu trzeba to zrobic dobrze. Problem polega na tym, ze w wiekszosci firm rozwiazania informatyczne sa tylko srodkiem do celu i nie zawsze ma sie nieograniczone zasoby jak:
a) czas
b) pieniadzeJak tak czasem czytam Twoje wypociny w tematach z dzialu kariera / inzynieria oprogramowania to mam wrazenie, ze jestes jakims ewenementem na skale Polski albo nawet i calego swiata. Jedyny sluszny programista, ktory robi wszystko zgodnie z fachem, nie przeciaga terminow, zespol IT go czci, a biznes jest w nim po uszy zakochany. A to wszystko za sprawa jednego, prostego tricku tzn wystarczy robic dobrze.
Pracuje w zespołach które wyznają podobne wartości - tylko większość z nich się nie udziela tutaj na forum.
Ja po prostu pisze jak projekty wyglądają u mnie -nie widzę w tym nic dziwnego.
Merytorycznie o kodzie się nie wypowiem, chociaż rodzi się wątpliwość jak 2 osobne metody zamknąć w transakcję, bo przecież sprawdzenie i zapis są rozciągnięte w czasie - nie wiem, nie moja bajka.
Jeżeli chodzi o komunikację, to istotą są użyte argumenty i "jakość dyskusji". Z niektórymi osobami rozmowa wygląda jak z przedszkola i zamiast prowadzić do znalezienia optymalnego rozwiązania idzie w kierunku "wygrania dyskusji". W takich przypadkach warto pamiętać, że zrobić "jak każą" jest często bardzo złośliwym działaniem, ale w toksycznych środowiskach się sprawdza, bo możesz tracić ile chcesz czasu na pyskówki o niczym, na koniec i tak zrobisz jak każą, więc spokojnie można pominąć ten etap. Jak jest to szczególnie głupie, to i tak szybko wróci i poprawa zużyje mniej czasu i co ważniejsze "softskillowej many", niż kopanie się z koniem.
Uwielbiam bayesian opitimizations, trzeba samplować tam gdzie jest największe uncertainty, niepewność.
Jeśli niepewność będzie niska to jesteśmy bardzo blisko rzeczywistego rozwiązania.
Jako, że jesteśmy na forum programistów to uncertainty jest badane przez std nad danym punktem, zależy oczwyiście nam na niskiej tej wartości, wtedy acquistions function jest w tym miejscu niska, a maximum tej funkcji jet punkt, który chcemy samplować czyli chcemy sprawidzić.
Nie jest. Co ci z tej dodatkowej warstwy abstrakcji? Jak będziesz chciał zmienić EF Core na NHibernate czy jakiegoś Dappera to i tak masz do przebudowy cały model domenowy wiec od groma roboty. Jak ktos mysli ze zmiana orm-a w enterprajsowej aplikacji to napisanie nowych configow dla encji i przepiecie implementacji w kontenerze DI to gratuluje poczucia humoru. Przepisywalem tak z NHibernate na Dapper i dodatkowa warstwa abstrakcji nic nie dala bo filozofia uzywania ORMa jest inna w kazdym ORMie.
No, ja słyszałem od pewnych osób że repo nad EFC jest super, bo im się przydało gdy bodajże 6 raz zmieniali bazke w projekcie
:D
Błędnie zakładasz że nie ma tu twojego team leadera, wklejając tu kawałek kodu oddanego do review.
rjakubowski napisał(a):
Czesc chlopaki.
Mam problem troche - nie ogarniam logiki wedle ktorej moj TL pisze kod przez co jak dostaje od niego review to mam srogie WTF.
Mam wrazenie, ze zawsze z najmniejszej glupoty, jak np jakis prosty check bo bazy w EF Core chlop tak mocno komplikuje sprawy, ze az mam z nim zwarcia.Czy w takich sytuacjach odpuszczacie i potakujecie jak baranki czy gryziecie do krwi, bo do chlopa nie docieraja pewne argumenty...?
Chetnie poczytam Wasze opinie,
Serdecznie pozdrawiam.
Mam wrażenie, że tytuł wątku nie odpowiada treści posta, w sensie pytanie, na które chcesz otrzymać odpowiedź nie jest tym, które zadałeś.
Ja zazwyczaj dostosowuję się do tego, co TL mi pisze w review, bo uwagi zazwyczaj odnoszą się tego, co kod ma robić. a on to wie lepiej. Jest po prostu bliżej produktu niż ja, więcej wymagań pamięta - pewnie dlatego, ze sam je spisywał.
Jeśli chodzi o aspekty techniczne, to też nie pamiętam, żeby mi ktoś kiedykolwiek w review zaproponował zrobienie czegoś głupiego, zazwyczaj są to warte zastosowania rzeczy.
markone_dev napisał(a):
Nie jest. Co ci z tej dodatkowej warstwy abstrakcji? Jak będziesz chciał zmienić EF Core na NHibernate czy jakiegoś Dappera to i tak masz do przebudowy cały model domenowy wiec od groma roboty. Jak ktos mysli ze zmiana orm-a w enterprajsowej aplikacji to napisanie nowych configow dla encji i przepiecie implementacji w kontenerze DI to gratuluje poczucia humoru.
Tu chyba problemem jest właśnie enterprajsowość aplikacji. W nich zazwyczaj czają się różne enterprajsowe pomysły, typu wrapper na ORMa udający API innego ORMa, który z kolei jest opakowany w biedarepozytoria. A, że domena jest anemiczna, a logika posiana w jakichś serwisach i okazyjnie mapperach, to faktycznie zmiana ORMa pociągnęłaby za sobą przepisanie prawie wszystkiego, no bo nie mamy chociażby tej logiki domenowej, która taką zmianę mogłaby przetrwać.
Przepisywalem tak z NHibernate na Dapper i dodatkowa warstwa abstrakcji nic nie dala bo filozofia uzywania ORMa jest inna w kazdym ORMie.
Dapper to nie ORM, i chyba nikt go nie reklamuje, że jest w stanie bogatą domenę (grafy prawidłowo enkapsulowanych obiektów) zapisać i odczytać z bazy.
markone_dev napisał(a):
Po prostu model domenowy dla EF Core ma inne wymagania niż model dla Dappera czy NHibernate'a więc dla rzeczy bedzie inaczej wyglądał.
W sensie już czterokrotnie przepisany EF wciąż nie umie w zewnętrzną konfigurację, i trzeba dekorować klasę atrybutami, a powiązania n do n wymagają klasy pośredniej?
Jedynym rozwiązaniem jest posiadanie dwóch modeli domenowego i persystencji ale nikt tak nie robi.
Uważałbym z takimi obelgami, ktoś się się może obrazić za nazwanie go nikim. :P
Tak naprawdę, to jest chyba jedyne sensowne podejście w przypadku bogatego modelu domenowego, bo nawet używając prawdziwego ORMa, będziemy musieli się dostosowywać do jakichś jego dziwactw typu virtual.
somekind napisał(a):
Dapper to nie ORM, i chyba nikt go nie reklamuje, że jest w stanie bogatą domenę (grafy prawidłowo enkapsulowanych obiektów) zapisać i odczytać z bazy.
Micro ORM ale nie chodzi tu o semantykę, tylko zmianę technologii używanej do persystencji modelu domenowego. Problem Dappera jest taki, że nie lubi się bardzo z prywatnymi polami i setterami. W przypadku, gdy tak jak piszesz masz bogatą domenę z wieloma typami a nie tylko proste stringi i inty to zaczyna się czarowanie żeby to ręcznie poskładać.
somekind napisał(a):
W sensie już czterokrotnie przepisany EF wciąż nie umie w zewnętrzną konfigurację, i trzeba dekorować klasę atrybutami, a powiązania n do n wymagają klasy pośredniej?
Wystarczy sobie popatrzeć jak obsługiwana jest współbierzność w NHibernate i EF Core, żeby zobaczyć, że zmiana z jednego na drugi wymaga zmiany typu pola na modelu. Do tego EF Core wciąż ma problem z inicjowaniem prywatnych backing fields jak kolekcje przy włączonym lazy loadingu i potem żeby coś sprawdzić na kolekcji (jakiś niezmiennik) robić hacki w postaci items.Count żeby zainicjować kolekcję. Jak się pisze TODO listę to faktycznie zmiana z A na B będzie trywialna, ale przy złożonych domenach, gdzie chcemy enkapsulować logikę na poziomie modelu już nie będzie tak prosto żeby zmienić A na B
somekind napisał(a):
Tak naprawdę, to jest chyba jedyne sensowne podejście w przypadku bogatego modelu domenowego, bo nawet używając prawdziwego ORMa, będziemy musieli się dostosowywać do jakichś jego dziwactw typu virtual.
Dlatego trzeba sobie odpowiedzieć na pytanie czy taka zabawa w abstrakcje nad DbContext czy ISession czy osobne modele na poziomie warstwy persystencji ma sens.
Posiadanie dwóch modeli ma jak najbardziej sens w sytuacji gdy integrujemy się z zewnętrznym systemem, który ma inny model domenowy od naszego i nie mamy nad nim żadnej kontroli. Dlatego tworzymy sobie wtedy warstwę Anti-Corruption Layer gdzie mapujemy dane z jednego modelu na drugi bo nie chcemy by model jakiegoś SAPa czy Salesforca zaśmiecał nam naszą domenę.
Robienie takiej samej abstrakcji na swojej bazie danych w większości sytuacji nie ma sensu bo czas pracy włożony w utworzenie takiej ACL i jej utrzymywanie w czasie tylko dlatego że może za 3 lub 4 lata zmienimy SQL Server na Mongo albo Cassandre jest nieuzasadnionym kosztem. Ale to moje zdanie i nie trzeba się z nim zgadzać :P
Jak czytam takie posty jak wyżej to zastanawiam się jaki argument ktoś miałby postawić, żeby kogoś przekonać że jeśli się zrobi odpowiednie odseparowanie logiki od persystencji, to praktycznie wszystko o czym mówicie jest szczegółem implementacyjnym.
Chyba będę musiał kiedyś przysiąść na tydzień i napisać taki projekt, który prezentuje to w sensowny sposób.
W sensie już czterokrotnie przepisany EF wciąż nie umie w zewnętrzną konfigurację, i trzeba dekorować klasę atrybutami, a powiązania n do n wymagają klasy pośredniej?
No to chyba masz bardzo stare informacje. W EfCore można już obecnie wszystko skonfigurować zewnętrznie a nawet wiecej niż na atrybutach. Powiąznia n do n bez klasy pośredniej również działaja i to nawet by convetion, oczywiście można je dodatkowo skonfigurować. Nie musimy mieć nawet żadnych publicznych seterów.Jedyne czego wymaga EF od obietku biznesowego który może znajdować się w zupełnie innej warstwie i o EF nie wiedzieć to pusty prywatny konstruktor , ale od tego raczej nie odejdziemy ze względu na ogarniczenie w samym .NET. Problemem który powinni rozwiązać to kompletnie nieoptymalne includy i dodanie natywnej możliwości zbudowania całego drzewa obiektu bez konieczności includowania wszystkich relacji. Skoro wyznaczyłem swój agregate root to chyba nie jestem debilem i nie zakroiłem go tak żeby pobierać pół bazy do pamięci czemu nie mogę więc zrobić coś w stylu Order.FetchAsync(x=> x.Id = x) i nie bawić się w jakieś extension methody z cała listą includów albo własne rozwiązania korzystające z internali EF żeby rekurencyjnie wyznaczyć wszystkie potrzebne includy...