Czy ten kod jest zgodny z zasadami SOLID?

0

Witam serdecznie.
Ostatnio zagłębiam się w tematykę zasad SOLID i postanowiłem napisać cokolwiek żeby sprawdzić czy dobrze to rozumiem i czy w praktyce jestem w stanie zastosować te zasady.
W związku z tym ma prośbę. Czy możecie "rzucić okiem" na ten kod i powiedzieć czy waszym zdaniem jest on zgodny z zasadami SOLID ?
Bardzo proszę o wytykanie wszelkich błędów i sugerowanie co należy zrobić inaczej.
Największy problem mam z interfejsami. Jakoś nie jestem w stanie intuicyjnie znaleźć dla nich zastosowania.

Na warsztat wziąłem NIP i kod poczty, bo to chyba niezły przykład do testowania.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Text.RegularExpressions;

namespace Utility
{
    public enum Tytuly { Pan, Pani, Panna }
    public enum CountryCode { PL, EN, DE }
    public enum TypAdresu { AdresDoKorespondencji, AdresZamieszkania, AdresSiedziby, AdresDostawy, AdrsDoFaktury }

    public abstract class Osoba
    {
        public Tytuly Tytul { get; set; }
        public string Imie { get; set; }
        public string Nazwisko { get; set; }
        BaseAdres[] Adresy { get; set; }
    }

    public abstract class Firma
    {
        public string NazwaSkrot { get; set; }
        public string NazwaPelna { get; set; }
        BaseAdres[] Adresy { get; set; }
        NumerNIP Nip { get; set; }
    }

    public abstract class KlientOsoba : Osoba
    {
        public NumerNIP Nip { get; set; }
    }

    public abstract class BaseAdres
    {
        public TypAdresu Typ { get; set; }
        public string Miejscowosc { get; set; }
        public string Ulica { get; set; }
        public string NrDomu { get; set; }
        public string NrLokalu { get; set; }
        public string Poczta { get; set; }
        public KodPocztowy KodPocztowy { get; set; }
    }

    interface IValidatable
    {
        bool IsValid();
    }

    public abstract class KodPocztowy : IValidatable
    {
        public string Kod { get; set; }
        public abstract bool IsValid();
        public static bool FormatCorrect(string format, string kodPoczowy)
        {
            Regex rgx = new Regex(format);
            Match match = rgx.Match(kodPoczowy);
            return match.Success;
        }
    }

    public class KodPocztowyPL : KodPocztowy
    {
        string PostalCodeRGX = @"^\b{2}-\b{3}";
        public override bool IsValid()
        {
            return (FormatCorrect(PostalCodeRGX, this.Kod));
        }
    }

    public class KodPocztowyEN : KodPocztowy
    {
        string PostalCodeRGX = @"^[A-Z][A-Z]\d+$";
        public override bool IsValid()
        {
            return (FormatCorrect(PostalCodeRGX, this.Kod));
        }
    }

    public abstract class NumerNIP : IValidatable
    {
        public NumerNIP() { }
        public NumerNIP(string nip) { Nip = nip; }
        public string Nip { get; set; }
        public abstract bool IsValid();
        protected abstract bool IsValidChecksum(string nip);
        internal static bool IsValidFormat(string[] format, string nrNIP)
        {
            bool isCorrect = false;
            foreach (string s in format)
            {
                Regex rgx = new Regex(s);
                Match match = rgx.Match(nrNIP);
                if (match.Success)
                {
                    isCorrect = true;
                    break;
                }
            }
            return isCorrect;
        }
    }

    public class NumerNIP_PL : NumerNIP
    {
        public NumerNIP_PL() : base() { }
        public NumerNIP_PL(string n) : base(n) { }

        string[] NipFormat = {
            @"^PL\d{3}-\d{3}-\d{2}-\d{2}$",
            @"^PL\d{3}-\d{2}-\d{3}-\d{2}$",
            @"^PL\d{3}-\d{2}-\d{2}-\d{3}$",
            @"^PL\d{10}$",
            @"^\d{3}-\d{3}-\d{2}-\d{2}$",
            @"^\d{3}-\d{2}-\d{3}-\d{2}$",
            @"^\d{3}-\d{2}-\d{2}-\d{3}$",
            @"^\d{10}$"
        };
        protected override bool IsValidChecksum(string nip)
        {
            return true; //TODO Weryfikacja sumy kontrolnej NIP dla PL
        }

        public override bool IsValid()
        {
            return (IsValidFormat(NipFormat, this.Nip) && IsValidChecksum(this.Nip));
        }
    }

    public class NumerNIP_EN : NumerNIP
    {
        string[] NipFormat = {
            @"^EN\d{3}-\d{3}-\d{2}-\d{2}$",
            @"^EN\d{3}-\d{2}-\d{3}-\d{2}$",
            @"^EN\d{3}-\d{2}-\d{2}-\d{3}$",
            @"^EN\d{10}$"
        };

        protected override bool IsValidChecksum(string nip)
        {
            return true; //TODO Weryfikacja sumy kontrolnej NIP dla EN
        }
        public override bool IsValid()
        {
            return (IsValidFormat(NipFormat, this.Nip) && IsValidChecksum(this.Nip));
        }
    }
}

```
2

Wg mnie nie jest z bardzo prostej przyczyny. Twoje klasy (Nip_EN, Nip_PL i te z kodami pocztowymi) łamią zasadę DRY. Spójrz, że jedyna różnica między nimi to tylko format. Powinieneś mieć tylko jedną klasę do NIP i tylko jedną klasę do Kodu pocztowego. W jakiś inny sposób powinieneś zatroszczyć się o sprawdzenie formatu. To może być dodatkowa rodzina klas, np:

abstract class Formatter
{
    public abstract string GetFormat();
}

class PostalCodePLFormatter: Formatter
{
   public string GetFormat()
   {
      return "format kodu";
   }
}
1

Najłatwiej to chyba dodać metodę do Formatter: Validate. Moim zdaniem nie zepsuje to zasady SRP, bo klasa daje format i sprawdza, czy przekazany ciąg na podstawie tego formatu jest właściwy. Czyli cały czas zajmuje się tylko formatem danych.

1

Trudno powiedzieć, bo zasady SOLID dotyczą programowania obiektowego, a ten kod jest proceduralny.
Na przykład, przechowywanie danych i ich walidacja powinny być oddzielone. Nie ma sensu dawanie możliwości utworzenia błędnego obiektu, w przypadku próby czegoś takiego konstruktor powinien rzucić wyjątek.
Na pewno nie łamie ISP, bo nie ma tu żadnych interfejsów.

0

Dziękuję za zwrócenie uwagi na separacje danych. Nie jestem pewien czy dobrze rozumiem konsekwencje tej zmiany. Czy to oznacza że w klasie np."Firma" nip powinien być polem typu string, a klasa NIP powinna zajmować się tylko walidacją i dostać nip jako parametr np w konstruktorze lub jako parametr metody, ale nie służyć do trzymania tej informacji ? Czy może źle to zrozumiałem.

1

Chodzi o to, abyś nie mógł utworzyć obiektu klasy "NIP" z niepoprawnym NIPem. Więc wartość NIPu można np. sprawdzić w konstruktorze i np. rzucić wyjątek jeśli jest niepoprawna.
A jeśli chcesz trzymać NIP jako string, to możesz napisać NIPValidator do sprawdzania wartości tego stringa przed jego ustawieniem w klasie Firma. Tylko zdecyduj się, czy chcesz modelować coś obiektowo, czy pisać DTO + walidatory do nich.

0

Podejście obiektowe chyba będzie lepsze, tylko że nie jestem pewien jak to zrobić w tym przypadku, bo:

  1. Przecież zdarzają się osoby które mają np pesel poprawny (w sensie prawidłowy) a walidatory wywalają błąd. Bo jakiś urzędnik coś źle podał i taka osoba ma papier z jakiegoś urzędu że tak ma być. Więc podejście, że nie utworzy się obiektu jeżeli walidacja będzie negatywna, chyba tu się nie sprawdzi.Bo można założyć, że z nipem może być tak samo (niby błędny a jednak tak ma być).
    Niektóre osoby nie mają pesela i w formularzach gdzie jest miejsce na pesel wpisują tylko datę urodzenia. No ale taki format można oprogramować, oczywiście z założeniem że nie sprawdza się wtedy sumy kontrolnej.
  2. Np w przypadku nipu inny jest algorytm sprawdzania sumy kontrolnej w zależności od kraju. Jeżeli podamy same cyfry to nie wiemy jakim algorytmem sprawdzać sumę kontrolną. Więc jeżeli byłby obiekt z walidacją w konstruktorze, to podczas tworzenia obiektu już musimy wiedzieć jaki tokraj i którego algorytmu użyć.
  3. A co w przypadku wczytywania danych z jakiegoś pliku czy innego źródła danych ? Czy nie lepiej jest utworzyć obiekt i później sprawdzać czy nip jest poprawny, dając możliwość jego korekty lub podania dodatkowych informacji ?

Jeżeli moje obiekcje wynikają, ze złego podejścia do problemu to proszę mnie poprawić bo chciałbym zrozumieć na czym polega mój błąd.
Z góry dziękuję.

0
Decho napisał(a):

Podejście obiektowe chyba będzie lepsze, tylko że nie jestem pewien jak to zrobić w tym przypadku, bo:

  1. Przecież zdarzają się osoby które mają np pesel poprawny (w sensie prawidłowy) a walidatory wywalają błąd. Bo jakiś urzędnik coś źle podał i taka osoba ma papier z jakiegoś urzędu że tak ma być. Więc podejście, że nie utworzy się obiektu jeżeli walidacja będzie negatywna, chyba tu się nie sprawdzi.

O nieunikalnych peselach słyszałem, ale o niepoprawnych jeszcze nie. Ale zakładając, że to, co napisałeś jest prawdą, to w ogóle implementacja jakiejkolwiek walidacji jest bezcelowa, a ten wątek nie ma sensu. Nie sądzisz? :)

Niektóre osoby nie mają pesela i w formularzach gdzie jest miejsce na pesel wpisują tylko datę urodzenia. No ale taki format można oprogramować, oczywiście z założeniem że nie sprawdza się wtedy sumy kontrolnej.

Najlepiej w ogóle zrobić jedno pole, w którym user wpisuje nagłówki i treść HTTP. tak ma Fiddler i to jest najlepsze możliwe GUI. :P

A nie można po prostu dać drugiego pola na datę a PESEL nieobligatoryjny zamiast tak kombinować?

  1. Np w przypadku nipu inny jest algorytm sprawdzania sumy kontrolnej w zależności od kraju. Jeżeli podamy same cyfry to nie wiemy jakim algorytmem sprawdzać sumę kontrolną. Więc jeżeli byłby obiekt z walidacją w konstruktorze, to podczas tworzenia obiektu już musimy wiedzieć jaki tokraj i którego algorytmu użyć.

NIP istnieje tylko w Polsce. Może inne kraje mają jakieś swoje inne numery podatkowe, ale to są w takim razie inne numery i tak powinny być rozumiane. Z oddzielnymi zasadami walidacji wybieranymi na podstawie podanego przez użytkownika kraju.

  1. A co w przypadku wczytywania danych z jakiegoś pliku czy innego źródła danych ? Czy nie lepiej jest utworzyć obiekt i później sprawdzać czy nip jest poprawny, dając możliwość jego korekty lub podania dodatkowych informacji ?

Tu się zaczyna filozofia i semantyka. Ogólnie w programowaniu obiektowym, obiekt to byt, który ma stan (dane) i zachowanie (funkcje). Jeśli nie ma funkcji, tylko dane, to to nie jest obiekt tylko rekord/struktura danych. Niestety nie mamy (przynajmniej w C#) specjalnego słowa kluczowego do definiowania rekordów, więc musimy używać tego samego, co do definiowania klas "pełnoprawnych" obiektów. Żeby było trudniej, na tego typu struktury bez logiki istnieje dość powszechne określenie DTO czyli Data Transfer Object. Ogólnie bajzel nieziemski panuje w nomenklaturze.

I teraz, do wczytywania danych z pliku, przesyłania przez sieć czy zapisywania do bazy używasz DTO, a do realizowania jakiejś logiki biznesowej pełnoprawnych obiektów (encji w narzeczu DDD). Możesz napisać sobie jakieś walidatory do danych z DTO, które przyjmą string i zwrócą true/false albo jakąś listę błędów walidacji, to się praktykuje - chociażby do walidacji danych wprowadzanych do aplikacji przez użytkownika. Ale z Twojego pytania wywnioskowałem, że skoro chcesz mieć KodyPocztowe i Nipy jako obiekty, a nie DTO, to nie o takie podejście Ci chodzi. Dlatego napisałem, że obiekt powinien być być zawsze w poprawnym stanie, a weryfikację danych należy przeprowadzać w konstruktorze.

0
somekind napisał(a):

O nieunikalnych peselach słyszałem, ale o niepoprawnych jeszcze nie. Ale zakładając, że to, co napisałeś jest prawdą, to w ogóle implementacja jakiejkolwiek walidacji jest bezcelowa

Można sprawdzać i wyświetlać ostrzeżenie o braku lub niepoprawnym PESELu, ale nie może to blokować wypełnienia czy wysłania formularza.

Niektóre osoby nie mają pesela i w formularzach gdzie jest miejsce na pesel wpisują tylko datę urodzenia.

O ile mi wiadomo, od jakiegoś roku nie wydaje się już PESELu cudzoziemcom, jak to do tej pory miało miejsce w przypadku osób nie mających polskiego obywatelstwa ale z kartą stałego pobytu. Osoby takie mają się posługiwać numerem paszportu jeśli gdziekolwiek mają podać PESEL (chyba że się na PESEL jeszcze załapali).

NIP w przypadku osób fizycznych też jest opcjonalny - był okres że dawali go każdemu chyba w momencie rozpoczęcia pracy, obecnie dopiero gdy ktoś zarejestruje działalność gospodarczą.

0
somekind napisał(a):

I teraz, do wczytywania danych z pliku, przesyłania przez sieć czy zapisywania do bazy używasz DTO, a do realizowania jakiejś logiki biznesowej pełnoprawnych obiektów (encji w narzeczu DDD). Możesz napisać sobie jakieś walidatory do danych z DTO, które przyjmą string i zwrócą true/false albo jakąś listę błędów walidacji, to się praktykuje - chociażby do walidacji danych wprowadzanych do aplikacji przez użytkownika. Ale z Twojego pytania wywnioskowałem, że skoro chcesz mieć KodyPocztowe i Nipy jako obiekty, a nie DTO, to nie o takie podejście Ci chodzi. Dlatego napisałem, że obiekt powinien być być zawsze w poprawnym stanie, a weryfikację danych należy przeprowadzać w konstruktorze.

Dokładnie. Chociaż kody pocztowe i pesel czy nip to tylko przykład na którym chciałem przetestować poprawność zrozumienia zasad solid i użycia interfejsów.
Równie dobrze za przykład mógłby posłużyć nr konta bankowego który ma sumę kontrolną, aczkolwiek w Polsce jest inny format numerów kont bankowych niż w innych krajach i inny jest algorytm liczenia sumy kontrolnej.

Chodziło im o stworzenie obiektu i oprogramowanie walidacji w przypadku gdy obiekt ten może mieć różną strukturę i zachowanie (w sensie algorytm walidacji np), z zachowaniem zasad, aby móc ten kod wykorzystać wiele razy, bez modyfikacji, łatwo go rozbudować itd. (czyli to o czym mówią zasady SOLID). Więc pomyślałem, że obiekt który będzie miał pewne wspólne rzeczy, a po nim (po klasie oczywiście) będą dziedziczyły inne obiekty, które będą miały specyficzne dla siebie reguły. Dlatego utworzyłem klase NIP i po nie dziedziczyła klasa NIP_PL, ale to podejście miało wadę że nie oddzieliłem warstwy danych od zachowania obiektu. Teraz siedzę nad tym i w wolnej chwili postaram się zgodnie w Waszymi wszystkimi uwagami utworzyć obiekt np konto bankowe z walidacją i z zachowaniem zasad solid.

O zasadach solid i interfejsach czytałem i analizowałem przykłady z tych stron, ale chciałem coś bardziej (przynajmniej dla mnie) skomplikowanego zrobić.
https://www.p-programowanie.pl/paradygmaty-programowania/zasady-solid/
https://www.benedykt.net/2012/04/30/dependency-inversion-principle-czyli-co-powinno-zalezec-od-czego/
http://plukasiewicz.net/CSharp_dla_poczatkujacych/Interfejsy
Oczywiście mogłem coś opacznie źle zrozumieć lub nie zrozumieć.
Jak napiszę kod do numeru bankowego to go tu wrzucę i poproszę o ocenę.

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