Zasady SO z SOLID - prośba o sprawdzenie.

0

Cześć,

Próbuję zrozumieć i nauczyć się stosować zasady SOLID.

Mam taki fragment kodu:

public class Post
{
	public int id {get; private set;}
	public string content {get; set;}

	public Post(int id, string content)
	{
		this.id = id;
		this.content = content;
	}
	
	public Post(string content)
	{
		this.content = content;
	}	
}

public interface IDbConnector
{
	Post GetPost(int postId);
	int AddPost(string content);
	void DeletePost(Post post);
	void UpdatePost(Post post)
}

public class PostManager : IDbConnector
{	
	private IDbConnector dbConnector;
	
	public PostManager(IDbConnector dbConnector)
	{
		this.dbConnector = dbConnector;
	}
	
	public Post GetPost(int postId)
	{
		Post post = dbConnector.GetPost(postId);
		return post;
	}
	
	public int AddPost(string content)
	{
		return dbConnector.AddPost(content);
	}
	
	public void DeletePost(Post post)
	{
		dbConnector.DeletePost(post);
	}
	
	public void UpdatePost(Post post)
	{
		dbConnector.UpdatePost(post); 
	}
}

Mam dwa pytania:

  1. Czy dobrze myślę, że klasa PostManager łamie zasadę pojedynczej odpowiedzialności?
    Jeżeli tak to jak powinna wyglądać jej implementacja?

  2. Czy rozwiązanie z wstrzykiwaniem IDbConnector jest poprawnym rozwiązaniem, aby zasada otwarte-zamknięte była spełniona? (z założenia PostManager może dostawać różne implementacje dbConnectora - np. w zależności od tego czy zapisujemy bezpośrednio do bazy danych czy np, przez jakieś API)?

0

Czy dobrze myślę, że klasa PostManager łamie zasadę pojedynczej odpowiedzialności?

gdzie?

Czy rozwiązanie z wstrzykiwaniem IDbConnector jest poprawnym rozwiązaniem, aby zasada otwarte-zamknięte była spełniona?

imo tak

0
WeiXiao napisał(a):

Czy dobrze myślę, że klasa PostManager łamie zasadę pojedynczej odpowiedzialności?

gdzie?

No właśnie nie jestem pewny czy dobrze rozumiem tę zasadę...
Jako "opis" podawane jest "klasa ma mieć wyłącznie jeden powód do modyfikacji".
Tutaj wydaje mi się, że ma więcej.
Przykładowo :

  • Zmiana sposobu tworzenia nowego posta (np jeżeli oprócz treści, będziemy chcieli zapisać jeszcze załączniki)
  • Zmiana sposobu modyfikacji (użytkownik będzie musiał podać powód modyfikacji)

Z drugiej strony, tworzenie oddzielnych klas dla tych działań wydaje się mało racjonalne chyba...

4

@hipekk w takiej sytuacji błąd jest w innym miejscu ;) Błędem jest przekazywanie jako parametru string content zamiast obiektu PostContent który można wygodnie rozszerzyć jeśli dojdą nam nowe dane.

5

Klasy tupu Manager raczej nie świadczą dobrze o jakości kodu. Do perzystencji danych należy zastosować wzorzecz DAO/Repository, zaś w logice biznesowej używać rich domain model + konkretne UseCas'y czy serwisy biznesowe, np. SendMobileTransferUseCase.

0

@Shalom: szczerze mówiąc to coś mi nie pasowało z tym parametrem :)
A dobrze myślę, że w takim przypadku należy utworzy typ bazowy dla posta "podstawowego" (np. z samym tekstem), a następnie w miarę potrzeb nie modyfikować tego istniejącego tylko tworzyć nowe typy dziedzicząc z tego bazowego?

@scibi_92 a czy to co pokazałem (interfejs, plus klasa która po nim dziedziczy (dla konkretnego typu bazy danych)) nie jest właśnie implementacją wzorca Repository?
(poprawiłem nazwy metod i dodałem dziedziczenie, bo właśnie się zorientowałem, że o tym zapomniałem...)

1

Dla mnie kod, który wrzuciłeś jest OK (nie łamie mojego rozumienie S, O), poza przyjmowaniem tego string na poziomie repozytorium. Ryzyko, które widzę, to to, że jeżeli ktoś będzie dziedziczył po klasie Post, zrobi sobie np. PostWithImage, to bez problemu będzie mógł go przekazać do metody update, która prawdopodobnie tego nie ogarnie w całości.

0
Shalom napisał(a):

@hipekk w takiej sytuacji błąd jest w innym miejscu ;) Błędem jest przekazywanie jako parametru string content zamiast obiektu PostContent który można wygodnie rozszerzyć jeśli dojdą nam nowe dane.

OCP nie mówi, że trzeba robić coś co jest na tą chwilę nie potrzebne. Uważam że kłamiesz, ponieważ zasada OCP mówi też, że "przeciw działanie przedwczesnym abstrakcją jest tak samo ważne jak same abstrakcje".

scibi_92 napisał(a):

Klasy tupu Manager raczej nie świadczą dobrze o jakości kodu.

W tym konkretnym przykładzie, świadczy o tym, że ktoś źle nazwał tą klasę.

Do perzystencji danych należy zastosować wzorzecz DAO/Repository, zaś w logice biznesowej używać rich domain model + konkretne UseCas'y czy serwisy biznesowe, np. SendMobileTransferUseCase.

No nie wiem czy to jest potrzebne w tym konkretnym przypadku. Mnie by wystarczył "table gateway".

3

Uważam że kłamiesz

xD Zaręczam ci ze dużo wygodniej czyta i pisze się kod w którym masz typy czy PostId, PostContent zamiast int i string, a już szczególnie kiedy takich obiektów robi się więcej. Dodatkowo wyciąganie jakiegoś YAGNI w takim przypadku to idiotyzm. Nie ma tu żadnej dodatkowej "abstrakcji". Jest budowanie modelu domenowego.

0
Shalom napisał(a):

Uważam że kłamiesz

xD Zaręczam ci ze dużo wygodniej czyta i pisze się kod w którym masz typy czy PostId, PostContent zamiast int i string, a już szczególnie kiedy takich obiektów robi się więcej.

Nie na temat.

Dodatkowo wyciąganie jakiegoś YAGNI w takim przypadku to idiotyzm.

Zwłaszcza kiedy pisze o tym Robert C. Martin.

Nie ma tu żadnej dodatkowej "abstrakcji". Jest budowanie modelu domenowego.

Skoro tak, to gdzie masz ten Close w OCP? :D

4

Zwłaszcza kiedy pisze o tym Robert C. Martin.

To on jest jakaś wyrocznią?

Skoro tak, to gdzie masz ten Close w OCP? :D

OCP nie mówi o zamykaniu modelu domenowego.

2
hipekk napisał(a):

Mam dwa pytania:

  1. Czy dobrze myślę, że klasa PostManager łamie zasadę pojedynczej odpowiedzialności?
    Jeżeli tak to jak powinna wyglądać jej implementacja?

Nie, bo ta klasa ma jeden powód do zmiany - zmianę sposobu przechowywania postów.

  1. Czy rozwiązanie z wstrzykiwaniem IDbConnector jest poprawnym rozwiązaniem, aby zasada otwarte-zamknięte była spełniona? (z założenia PostManager może dostawać różne implementacje dbConnectora - np. w zależności od tego czy zapisujemy bezpośrednio do bazy danych czy np, przez jakieś API)?

Ja tu widzę przede wszystkim, że PostManager może przyjąć sam siebie w zależnościach, a to już wygląda jak miejsce na potencjalny wybuch.

Moim zdaniem doszukiwanie się open-close w tym przykładzie jest mocno na siłę. Ale jak już zauważono, na pewno przyjmowanie sensowych obiektów zamiast stringów/intów ułatwia późniejszy rozwój.

0
scibi_92 napisał(a):

Zwłaszcza kiedy pisze o tym Robert C. Martin.

To on jest jakaś wyrocznią?

A to skoro nie jest wyrocznią to temat zamknięty, możemy się rozejść :D

Skoro tak, to gdzie masz ten Close w OCP? :D

OCP nie mówi o zamykaniu modelu domenowego.

Przykładowy obiekt z książki Martina na którym ilustruje problemy związane z OCP obejmuje zachowania oraz dane, więc można go zidentyfikować jako Domain Model. Dlaczego kłamiesz pisząc że "OCP nie mówi o zamykaniu modelu domenowego."?

5
BananowyMarek napisał(a):

Przykładowy obiekt z książki Martina na którym ilustruje problemy związane z OCP obejmuje zachowania oraz dane, więc można go zidentyfikować jako Domain Model. Dlaczego kłamiesz pisząc że "OCP nie mówi o zamykaniu modelu domenowego."?

o_O
Ale gdzie OCP mówi o zamykaniu jakiegokolwiek modelu? Przecież tutaj chodzi o to, żeby klasy/funkcje/metody itp. były otwarte na rozszerzenia i zamknięte na modyfikacje. To, że ktoś sobie stworzy PostId zamiast int ma się nijak do tematu.

0

@hipekk:

hipekk napisał(a):

Mam taki fragment kodu:

Może nie do końca w temacie, ale się wypowiem.
Używasz C#, więc tutaj modele bazodanowe powinno tworzyć się nieco inaczej - głównie ze względu na możliwość używania ORM'ów. Czyli:

  • propertisy publiczne
  • pusty konstruktor

Czyli:

public class Post
{
  public int Id {get;set;}
  public string Content {get;set;}
}

A jeszcze lepiej jakbyś stworzył jakiś bazowy model, który posiada tylko Id, a Post by z niego dziedziczył.

public interface IDbConnector
{
Post GetPost(int postId);
int AddPost(string content);
void DeletePost(Post post);
void UpdatePost(Post post)
}

To nie jest dobry interfejs. Ty tu nie tworzysz interfejsu connectora, tylko interfejs dla repozytorium. W dodatku repozytorium dedykowane tylko dla klasy Post. Poczytaj o wzorcu repozytorium i jak to powinno wyglądać. Wskazówka - jeśli używasz ORMa w stylu nHibernate lub EntityFramework, nie używaj już repozytorium. Bo te ORMy same w sobie są repozytoriami.

public class PostManager : IDbConnector
{
private IDbConnector dbConnector;

public PostManager(IDbConnector dbConnector)
{
this.dbConnector = dbConnector;
}

public Post GetPost(int postId)
{
Post post = dbConnector.GetPost(postId);
return post;
}

public int AddPost(string content)
{
return dbConnector.AddPost(content);
}

public void DeletePost(Post post)
{
dbConnector.DeletePost(post);
}

public void UpdatePost(Post post)
{
dbConnector.UpdatePost(post);
}
}

Ktoś już powiedział, że klasy "Manager" nie są dobrymi klasami. Z tego względu, że już sama nazwa sugeruje, że to klasy "bogowie". Po drugie tworzysz tutaj coś bardzo dziwnego. Tworzysz niby repozytorium, ale ni to repo, ni to wzorzec dekorator. W skrócie prawilne repozytorium powinno wyglądać mniej więcej tak:

public class PostRepository: IRepository<Post>
{
  IDbAccess db;

  public PostRepository(IDbAccess db)
  {
    this.db = db;
  }

  public int Add(Post p)
  {
    string sql = ... tworzysz sqla z parametrami, być może za pomocą IDbAccess, a na koniec:
   db.ExecuteSql(sql);
  }

  //i inne metody
}

Mam dwa pytania:

  1. Czy dobrze myślę, że klasa PostManager łamie zasadę pojedynczej odpowiedzialności?

Łamie, ale tylko ze względu na nazwę ;) Tzn. jest szansa, że zaraz złamie.

Przede wszystkim to, co zrobiłeś w IDbConnector nie jest tym, czym powinno być. Connector, a więc coś, co łączy się z bazą danych. W repozytoriach stosuje się raczej jakiś "DBAccess", czyli coś, co ma dostęp do bazy danych. Taka klasa powinna umieć wykonać zapytania SQL, ogarnąć transakcje i w zasadzie tyle. Powinna być zupełnie niezależna od modelu.

0

@Juhas:

Juhas napisał(a):

@hipekk:

hipekk napisał(a):

Mam taki fragment kodu:

Może nie do końca w temacie, ale się wypowiem.
Używasz C#, więc tutaj modele bazodanowe powinno tworzyć się nieco inaczej - głównie ze względu na możliwość używania ORM'ów. Czyli:

  • propertisy publiczne
  • pusty konstruktor

Czyli:

public class Post
{
  public int Id {get;set;}
  public string Content {get;set;}
}

Ee ale jak. Przecież można zrobić w EF tak:

public class Post 
{
    public int Id {get; protected set;}
  //Jeden konstruktor bezparametrowy protected czy tam private - dla EF
// a publiczny z parametrami
}

To nie byłoby lepsze podejście?

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