Pomoc w poprawieniu obecnej solucji

Hej,

Piszę sobie pewne API. Ogólny zarys systemu raczej nie jest istotny więc skupię się tylko na dwóch jego częściach: Posty (takie jak na forum) oraz Subskrypcja - dwa typy użytkowników: darmowi oraz płatni.

Wysyłam zapytanie do funkcji w PostsService przekazując 3 parametry: post, datę posta oraz userId.
Funkcja ma dokonać wielu sprawdzeń, między innymi:

• czy taki post istnieje
• jeśli nie istnieje, to czy istniał w przeszłości: jeśli tak, to trzeba go skopiować, stworzyć z aktualną datą oraz zapisać
• sprawdzić czy użytkownik ma prawo do tego aby dodać tego posta (czy jest darmowy czy nie)
• jeśli użytkownik ma płatną subskrypcję to wtedy wysyłam dodatkowe zapytanie do innego zewnętrznego API z którego korzystam
  Dodatkowo w części SubscriptionService dokonuje kilku innych operacji:
• jeżeli jest darmowy to sprawdzam tylko bazę danych
• jeśli jest płatny to odpytuje dodatkowo zewnętrzne API
• zwiększam licznik zapytań danego usera
• jeżeli user ma płatną subskrypcję to trzeba sprawdzić czy data jej wygaśnięcia jest większa niż dzisiejsza, jeśli NIE to znaczy że mu się skończyła i staje się user darmowym...

Wiem jest to zagmatwane. Próbuje to zakodować już od co najmniej tygodnia, mam spore problemy, podejrzewam że dlatego że nie umiem za dobrze projektować obiektowo.

Czy mógłby mi ktoś pomóc nakreślić jak to powinno ogólnie ze sobą wszystko współgrać?

To jakieś WebAPI?

Sprawdzanie uprawnień użytkowników najlepiej zrobić jakimiś atrybutami (Authorize). Natomiast całą resztę logiki musisz puścić dwoma potokami, innym dla płatnych i darmowych użytkowników. Przydać się tu może np. wzorzec strategia.
Więcej bez kodu trudno powiedzieć.

Tak to jest WebAPI, co do tych atrybutów... to niestety ale nie do końca o takie uprawnienia chodzi. User który wysyła zapytanie do funkcji już jest po autoryzacji.
Okay, dzięki za wzorzec, zobaczę jak to zaimplementować tym wzorcem.

A co do kodu: nie chciałbym go wrzucać...

Skoro dopiero sprawdzasz jego uprawnienia do jakiegoś zasobu, to jest przed autoryzacją, a nie po. Może wcześniej jest jakaś inna autoryzacja, ale nic nie stoi na przeszkodzie, żeby mieć dwie.

No sprawdzam jego subskrypcję - czy jest to płatny czy nie płatny i na tej podstawie udzielam mu dostępu do odpytywania zewnętrznego API (jakby jeszcze innego niż to moje).

W tym punkcie chodziło mi o sprawdzanie właśnie tej subskrypcji

• sprawdzić czy użytkownik ma prawo do tego aby dodać tego posta (czy jest darmowy czy nie)

Sprawdzenie czy użytkownik ma prawo == autoryzacja :)

Dobra okay, niech będzie że to jest autoryzacja. Robię zapytanie do DB, pobieram jego subskrypcję i na tej podstawie wykonuje dalszy kod.

Tylko mam takie problemy że coś piszę i potem się okazuje że przydało by się coś jeszcze zrobić z daną rzeczą :/
Przykładowo: w momencie gdy darmowy użytkownik doda nowego posta, to ten post nie ma wszystkich informacji - ma jedynie ich minimalną ilość (minimalną do sensownego pokazania tego posta), ale jeśli później płatny użytkownik zrobi zapytanie o ten sam numer post-a, to ja muszę tak czy siak odpytać to zewnętrzne API i pobrać te brakujące dane, a następnie je wsadzić do istniejącego posta....
Widzisz w czym ja mam problemy? :D 1500 różnych zależności IF-ów, sprawdzania itd. I ogólnie ja to mam już zakodowane jako tako i to działa, ale chciałbym po prostu zrobić to jakoś lepiej i nie umiem.

Oczywiście później jeszcze dojdzie wysyłka mailingu w odpowiednich miejscach oraz ustawianie schedulera za pomocą Quartz.net

Mi brakuje podstawowej informacji, co ma być efektem wywołania tej operacji z API? Opisałeś parametry, jakąś logikę, ale nie wiadomo do czego ma to wywołanie doprowadzić. Jak nie będziesz wiedział co chcesz osiągnąć, to będzie to trudne do realizacji ;P

Ma zostać zwrócony obiekt Post z różnego rodzaju informacjami.

Ogólna zasada to podzielić na mniejsze części i delegować do innych komponentów to czego nie umiemy zrobić sami ;-)
Taki PostService pewnie korzysta z innych komponentów: repozytoriów Użytkowników/Postów, interfejsu do zewnętrznego systemu etc.

Z perspektywy kodu mógłbyś to podzielić tak by logika była opisana za pomocą niewielkiej ilości operacji, a implementacja tychże operacji analogicznie.

Post queryPost(post,postDate,userId) {
	// techniczna weryfikacja parametrów, możliwy wyjatek -> InvalidParameters
	validateParameters(post,postDate,userId); 
	
	// pobranie usera, możliwy wyjątek -> UnknownUser
	u = getUser(userId);				
	
	// weryfikacja uprawnień -> UnauthorizedAccess
	checkSecurity(u);						  
	
	// pobranie Posta z repozytorium, możliwty wyjątek UnknownPost
	p = retrievePost(post,postDate);				  
	
	// podbijamy licznik użycia API
	u.incQueryCounter("queryPost");					  
	
	// dodatkowa logika, w zależności od tego co tam user ma 
	// zapytanie do zewnetrznego serwisu, aktualizacja cyklu zycia usera etc.
	handleExtraLogic(u,p)
	
	return p; 
}


void handlExtraLogic(User u, Post p) {
	if (u.hasPaidSubscription()) {
		extSystem.getPremiumContent("s3kr3t_key",u);
		p.enablePremiumContent();
		p.setContent(result);
		
	}
	p.increaseViewCounter();
	p.addViewedBy(u);
	u.charge(42); 
}

W tym co napisałeś jest taka linijka:

u.hasPaidSubscription()

Czyli funkcja HasPaidSubsciption jest w klasie User?? Ja obecnie to mam w serwisie SubscriptionService i wywołuje to mniej więcej tak:

var p = _subscriptionService.IsFreeUser(user);

lepiej to przenieść do user-a czy zostawić tak jak mam>?

Dobra...
wrzucam ten kod:

   public ResponseMessage<Models.WebApi.Post> NewFunction1(NewPost p, Guid userId)
        {
            var response = new Models.WebApi.Post();
            if (p.Date < DateTime.Now.Date)
            {
                return CreateResponseMessage(response, new List<string> { Errors.DateInPast });
            }
            var user = _userRepo.Get(userId);
            var post = _postRepo.GetPost(p.Number, p.Date.ToUniversalTime());
            var isFree = _subscriptionService.IsFreeUser(userId);

            if (post != null)
            {
                if (post.IsVerified)
                {
                    return PreparePostAssignToUser(post, user);
                }
                else if (!post.IsVerified && !isFree)
                {
                    // handle update of post for payed user!
                    // get from API
                    // update existing post with data!
                }
                else if (!post.IsVerified && isFree)
                {
                    // return the post
                }
            }

            if (isFree)
            {
                var existsInPast = CheckPast();
                if (existsInPast)
                {
                    post = NewPostObjFreeUser();
                    return PreparePostAssignToUser(post, user);
                }
                else
                {
                    return CreateResponseMessage(new Models.WebApi.Post(), new List<string> { Errors.PostNotFound });
                }
            }
            else
            {
                var subscriptionActive = ActualSubscription(user);
                if (subscriptionActive)
                {
                    post = GetPostFromApi();
                    _subscriptionService.IncreaseUserCalls(user.Id);
                    NewPostObjPayedUser();
                    PreparePostAssignToUser(post, user);
                }
                else
                {
                    var existsInPast = CheckPast();
                    if (existsInPast)
                    {
                        post = NewPostObjFreeUser();
                        return PreparePostAssignToUser(post, user);
                    }
                    else
                    {
                        return CreateResponseMessage(new Models.WebApi.Post(), new List<string> { Errors.PostNotFound });
                    }
                }
            }
            // return proper value. 
            return null;
        }

Oczywiście pierwsze co mi się nasuwa jak na to patrze to że ta funkcja jest po prostu długa... :( wydaje mi się też że jest cała zdupczona i że jest to świetny przykład amatorskiego kodowania :/

I dobrze Ci się wydaje. Polecam zapoznać się z zasadą pojedynczej odpowiedzialności i Null Object pattern ...

W repo do posta mam coś takiego:

return _session.QueryOver<Post>()
                   .WhereRestrictionOn(x => x.Number).IsInsensitiveLike(number)               
                   .OrderBy(x => x.DateUtc).Desc
                   .Take(1)
                   .List()
                   .FirstOrDefault();

jeśli go nie znajduje to zwraca mi null-a.

BTW: tak wiem, nie mam interfejsów... powinny być.

A jak wygląda uwierzytelnienia to jakiś Owin + OAuth? Tokeny?

EDIT:
Można by się pozbyć trochę kodu i wrzucić informacje o użytkowniku do ClaimsIdentity.

ne0 napisał(a):

W tym co napisałeś jest taka linijka:

u.hasPaidSubscription()

Czyli funkcja HasPaidSubsciption jest w klasie User?? Ja obecnie to mam w serwisie SubscriptionService i wywołuje to mniej więcej tak:

var p = _subscriptionService.IsFreeUser(user);

lepiej to przenieść do user-a czy zostawić tak jak mam>?

Na podstawie opisu założyłem, że jest tylko jedna data i jest porównywana z sysdate, dlatego wydało mi się właściwe umieścić tę operację w klasie User.

Można podejść do tego tak, że "free user" zmienia się na tyle często i ocena tego czy to jest "free user" jest złożona, że trzeba to wydzielić do osobnego serwisu. Może być tak, że zmienia się rzadko, np. użytkownik kupuje jakieś "doładowanie" raz na kilka miesięcy i wtedy data ważności konta ulga przesunięciu. Wówczas logika mogła by być na użytkowniku, bo ta klasa będzie miała atrybut "validTo", który pozwoli ocenić czy "user free" czy nie. Ten atrybut może być aktualizowany przez serwis zarządzający subskrypcjami raz na miesiąc, dzień etc. Z drugiej strony może być tak, że użytkownik może mieć wiele subskrypcji i zarządzanie nimi jest złożone. Dużo zależy od tego jak wygląda domena, kto tworzy subskrypcje. Wg mnie nie należy iść w kierunku rozsmarowanej odpowiedzialności (tzn. czasem subskrypcję aktualizuje user, czasem serwis).

Podczas rejestracji user dostaje wersje konta darmowego. Może kupić konto PRO i wtedy zmienia mu się subskrypcja. To wszystko (zmiana subskrypcji, jej update'y etc. oraz te kawałki z powyższego kodu są w SubscriptionService).

Ja tutaj miałbym też ogólnie pytanie do Was: co mogę konkretnie zrobić aby umieć lepiej implementować takie przypadki w myśl programowania obiektowego. Jak mogę podnieść swoje umiejętności ?