Pytanie odnośnie Single Responsibility Principle

Cześć, zagłębiając się w temat SOLID'nego kodu analizuję pierwszą zasadę SRP. W związku z tym napisałem kawałek kodu i rodzą mi się dwa pytania/tematy do rozmyśleń. Pierwsze pytanie brzmi następująco: Czy poniższa klasa łamie zasadę SRP ?

public class Pracownik()
{
public void WydrukujDanePracownika();
public void PobierzDanePracownika();
public void ObliczPensjePracownika();
}

Osobiście uważam, że na podstawie powyższego kodu nie jesteśmy w stanie tego stwierdzić, ponieważ nie wiemy, w jaki sposób implementowane są w/w metody. Wydaje mi się, że jeżeli metody te są implementowane za pomocą innych klas (klasa drukująca, pobierająca dane i obliczająca pensję) to zasada SRP jest spełniona, a klasa Pracownik posiada metody, które pozwolą wyeksponować funkcjonalność daną pracownikowi.

Drugie pytanie brzmi następująco: **Czy poniższy kod łamie zasadę SRP, bądź inną SOLID'ną ? **

public interface IDrukowanie
{
    void DrukujRaport(IModul modul);
}

public class GenerujRaportXml : IGeneratorRaportow
{
    public void GenerujRaport(IModul modul)
    {
        modul.TrescRaportu = "tresc raportu w formacie XML.";
    }
}

public class GenerujRaportText : IGeneratorRaportow
{
    public void GenerujRaport(IModul modul)
    {
        modul.TrescRaportu = "tresc raportu w formacie tekstowym.";
    }
}

public class DrukujRaportXml : IDrukowanie
{
    public void DrukujRaport(IModul modul)
    {
        var tekst = string.IsNullOrEmpty(modul.TrescRaportu)
                        ? "Brak tresci raportu."
                        : string.Format("Wydrukowano w formacie XML'owym {0}", modul.TrescRaportu);

        Console.WriteLine(tekst);
    }
}

public class DrukujRaportText : IDrukowanie
{
    public void DrukujRaport(IModul modul)
    {
        var tekst = string.IsNullOrEmpty(modul.TrescRaportu)
                        ? "Brak tresci raportu."
                        : string.Format("Wydrukowano w formacie tekstowym {0}", modul.TrescRaportu);

        Console.WriteLine(tekst);
    }
}

public enum JakiZapis
{
    Xml,
    Text,
};

public interface IFactory
{
    IDrukowanie UtworzObiektDrukowania(JakiZapis param);
    IGeneratorRaportow UtworzObiektGenerowania(JakiZapis param);
}

public class Factory : IFactory
{
    public IDrukowanie UtworzObiektDrukowania(JakiZapis param)
    {
        switch (param)
        {
            case JakiZapis.Text:
                return new DrukujRaportText();
            case JakiZapis.Xml:
                return new DrukujRaportXml();
            default:
                return null;
        }
    }

    public IGeneratorRaportow UtworzObiektGenerowania(JakiZapis param)
    {
        switch (param)
        {
            case JakiZapis.Text:
                return new GenerujRaportText();
            case JakiZapis.Xml:
                return new GenerujRaportXml();
            default:
                return null;
        }
    }
}

public interface IModul
{
    string TrescRaportu { get; set; }
}

public class Modul : IModul
{
    private readonly IFactory _factory;
    public Modul(IFactory factory)
    {
        _factory = factory;
    }

    public string TrescRaportu { get; set; }

    public void GenerujRaport(JakiZapis wyborTypuRaportu)
    {
        var obiekt = _factory.UtworzObiektGenerowania(wyborTypuRaportu);
        obiekt.GenerujRaport(this);
    }
    public void DrukujRaport(JakiZapis wyborTypuRaportu)
    {
        var obiekt = _factory.UtworzObiektDrukowania(wyborTypuRaportu);
        obiekt.DrukujRaport(this);
    }
}

class WyswietlenieProgramuNaEkran
{
    private static void Main()
    {
        var mojModul = new Modul(new Factory());
        mojModul.GenerujRaport(JakiZapis.Xml);
        mojModul.DrukujRaport(JakiZapis.Text);
        Console.ReadLine();
    }
}

Nie mówię, że to łamanie jakichś zasad, ale takie pytania mi się nasuwają:

1. Dlaczego klasa Modul dostaje w konstruktorze obiekt IFactory?
2. Dlaczego jej metody same tworzą obiekty usług, które operują na jej własnych danych?
3. Dlaczego jest jedna klasa Factory, a nie dwie?

Dzięki za zainteresowanie. Spróbuję odpowiedzieć na twoje pytania, długo się zastanawiałem, zanim na nie odpowiedziałem i nadal nie mam pełnej jasności w temacie.

Ad 1. Z tego co wyczytałem, według Single Responsibility Principle tworzenie obiektów to już pewna odpowiedzialność, więc utworzenie obiektu klasy Factory wewnątrz klasy Moduł było by złamaniem SRP. Ponadto jeżeli chcielibyśmy podmienić klasę Factory na inną, mamy tutaj taką możliwość. Dodatkowo konstruktor upewnia mnie, że klasa typu IFactory zostanie dostarczona obiektowi typu Modul, bez którego nie działają metody. Chyba, że widzisz jeszcze inne rozwiązanie ?

Ad 2. Jeżeli dobrze rozumiem pytanie to właśnie dlatego aby zachować Single Responsibility Principle. Ze względu na to, że metody w tej klasie mają różne odpowiedzialności, nie implementuję metod w tej klasie, tylko deleguję je do innych klas, odpowiadających za poszczególne implementacje. Dodatkowo, analogicznie do Ad1, przeniosłem odpowiedzialność za tworzenie obiektów do klasy Factory (przy ewentualnym dodaniu nowego typu raportu dodajemy tylko enuma i implementujemy go w klasach raportów, klasa moduł automatycznie będzie go obsługiwać).

Ad 3. Nie wiem jak odpowiedzieć na to pytanie, ale klasa Factory ma raczej jedną odpowiedzialność, tworzenie obiektów. Czy klasa Factory łamie SRP ? Czy powinny być dwie klasy typu Factory ?

Wydaje mi się, że nie za bardzo rozumiesz ideę fabryki. Tutaj sobie popatrz jak gość to zaimplementował: http://www.codeproject.com/Articles/328373/Understanding-and-Implementing-Abstract-Factory-Pa

Tak na marginesie złamałem zasadę OCP przy budowaniu klasy Factory. Odnośnie do twojej podpowiedzi siaradek to wg mnie przykład, który mi podałeś łamie zasady SOLID.

Po pierwsze, metoda CheckProducts() klasy PhoneTypeChecker wg mnie posiada trzy funkcjonalności:

• wybiera fabrykę,
• tworzy obiekty (Jeżeli chcesz w klasie tworzyć obiekty to to już jest odpowiedzialność),
• wyswietla na ekran wyniki.
  Czy nie powinna ona sprawdzać produkt, jak sama nazwa wskazuje (cokolwiek miałaby zawierać)? Wg tego przykładu, w przypadku, gdy chcesz dodać nową firmę, musisz modyfikować funkcję sprawdzającą produkt.

Po drugie funkcja CheckProducts(), podobnie jak u mnie łamie OCP. Nie widzę również analogii do mojego przykładu, możesz mi to wytłumaczyć ?

86Michal86 napisał(a):

Chyba, że widzisz jeszcze inne rozwiązanie ?

Owszem, widzę. Ja bym do klasy Modul przekazywał obiekty IDrukowanie oraz IGeneratorRaportow, a nie IFactory.

Ad 2. Jeżeli dobrze rozumiem pytanie to właśnie dlatego aby zachować Single Responsibility Principle. Ze względu na to, że metody w tej klasie mają różne odpowiedzialności, nie implementuję metod w tej klasie, tylko deleguję je do innych klas, odpowiadających za poszczególne implementacje.

Ok, chyba ja zadałem swoje pytanie w sposób niezrozumiały. Chodzi mi o to, że:

1. Klasa Modul zawiera jakieś dane.
2. Klasa implementująca np. IDrukowanie zajmuje się przetwarzaniem danych z klasy Modul.
3. Klasa Modul ma metodę, która wywołuje metodę obiektu IDrukowanie, i przekazuje samą siebie do przetworzenia.
   Dla mnie ostatni punkt to niepotrzebne zawirowanie, to nie jest moim zdaniem przejrzysta konstrukcja aplikacji.

Czy powinny być dwie klasy typu Factory ?

Ja do każdej rodziny obiektów zawsze robię oddzielną fabrykę. Wydaje mi się, że tworzenie obiektów IDrukowanie i tworzenie obiektów IGeneratorRaportow to jednak dwie różne odpowiedzialności.

Trochę zmieniłem nazewnictwo, aby było ono bardziej czytelne. Utworzyłem 2 statyczne fabryki (zasadę OCP zostawię sobie na później, póki co nie wiem jak to rozwiązać) i wcisnąłem je w funkcje znajdujące się w klasie Raport.

à propos Ad2. obecne rozwiązanie pozwala mi na użycie klas typu IDrukowanie i IGenerowanie na dowolnych obiektach typu IRaport (mogę utworzyć i wydrukować wszystko co posiada TrescRaportu, czyli np. w innym miejscu programu).

Funkcja GenerujRaport pozwoli mi ustawić obiekt, który przed chwilą utworzyłem (czyli np. podano mi TrescRaportu i typ raportu w formacie XML, więc funkcja GenerujRaport bierze obiekt, który przed chwilą powstał i opakowywuje TrescRaportu w format XML).

Drukowanie raportów pobiera również świeżo utworzony obiekt, ponieważ, w przypadku, gdybyśmy mieli więcej do wydrukowania niż TrescRaportu będę miał dostęp do pól tej klasy. Możnaby do DrukujRaport podać stringa TrescRaportu, ale co w przypadku większej ilości pól w klasie (np. pole sterujące marginesem górnym).

Już mi się kręci w głowie od tego kombinowania :) Może znów masz jakieś inne ciekawe rozwiązanie, które uprościłoby rozwiązanie tego problemu ? Z góry dzięki za podpowiedzi, nie rozwiązuje tego problemu w celach prywatnych, mam nadzieję, że rozmyślenia te przydadzą się również komu innemu.

public class GenerujRaportXml : IGeneratorRaportow
{
    public void GenerujRaport(IRaport raport)
    {
        raport.TrescRaportu = "tresc raportu w formacie XML.";
    }
}

public class GenerujRaportText : IGeneratorRaportow
{
    public void GenerujRaport(IRaport raport)
    {
        raport.TrescRaportu = "tresc raportu w formacie tekstowym.";
    }
}

public interface IDrukowanie
{
    void DrukujRaport(IRaport raport);
}

public class DrukujRaportXml : IDrukowanie
{
    public void DrukujRaport(IRaport raport)
    {
        var tekst = string.IsNullOrWhiteSpace(raport.TrescRaportu)
                        ? "Brak tresci raportu."
                        : string.Format("Wydrukowano w formacie XML'owym {0}", raport.TrescRaportu);

        Console.WriteLine(tekst);
    }
}

public class DrukujRaportText : IDrukowanie
{
    public void DrukujRaport(IRaport raport)
    {
        var tekst = string.IsNullOrWhiteSpace(raport.TrescRaportu)
                        ? "Brak tresci raportu."
                        : string.Format("Wydrukowano w formacie tekstowym {0}", raport.TrescRaportu);

        Console.WriteLine(tekst);
    }
}

public enum TypRaportu
{
    Xml,
    Text,
};

public static class DrukowanieRaportowFactory
{
    public static IDrukowanie UtworzObiektDrukujacyRaport(TypRaportu param)
    {
        switch (param)
        {
            case TypRaportu.Text:
                return new DrukujRaportText();
            case TypRaportu.Xml:
                return new DrukujRaportXml();
            default:
                return null;
        }
    }
}

public static class GenerowanieRaportowFactory
{
    public static IGeneratorRaportow UtworzObiektGenerujacyRaport(TypRaportu param)
    {
        switch (param)
        {
            case TypRaportu.Text:
                return new GenerujRaportText();
            case TypRaportu.Xml:
                return new GenerujRaportXml();
            default:
                return null;
        }
    }
}

public interface IRaport
{
    string TrescRaportu { set; get; }
}

public class Raport : IRaport
{
    public string TrescRaportu { get; set; }
    
    public void GenerujRaport(TypRaportu typRaportu)
    {
        var obiekt = GenerowanieRaportowFactory.UtworzObiektGenerujacyRaport(typRaportu);
        obiekt.GenerujRaport(this);
    }

    public void DrukujRaport(TypRaportu typRaportu)
    {
        var obiekt = DrukowanieRaportowFactory.UtworzObiektDrukujacyRaport(typRaportu);
        obiekt.DrukujRaport(this);
    }
}

class WyswietlenieProgramuNaEkran
{
    private static void Main()
    {
        var mojModul = new Raport();
        mojModul.GenerujRaport(TypRaportu.Xml);
        mojModul.DrukujRaport(TypRaportu.Text);
        Console.ReadLine();
    }
}