Czy dobrze uczę się tworzenia klas?

0

Witam! Uczę się programowania od niedawna i potrzebuję informacji zwrotnej czy w poprawny sposób modeluje klasy (aplikacje konsolowe). Dla przykładu wstawiam klasę jednego z moich programów (w podobny sposób robię inne). Co mógłbym poprawić? Może kod jest zbyt mało czytelny? Czy dobrze zrobiłem, że propertisy ustawiam oddzielnymi metodami sprawdzającymi poprawność danych? A i jeszcze jedno. Na razie roboczo gdy dane są niepoprawne rzucam po prostu wyjątkiem. W jaki sposób najlepiej, najpoprawniej dać użytkownikowi możliwość poprawy gdyby to on wprowadzał dane? Np. gdy poda złą nazwę sklepu, tworzyć metodę w tej samej klasie która będzie pozwalała skorygować mu błąd i wykorzystać ją w setShopName();? Czy może oddzielna klasa odpowiedzialna za możliwość korekty pomyłek użytkownika? Pytania pewnie banalne ale na początku drogi nie chcę wyrabiać złych nawyków.

using System;

namespace ShopManager
{
    public class Shop
    {
        public string ShopName { get; private set; }
        public string LocationId { get; private set; }
        public int Id { get; private set; }
        public bool IsOpen { get; }
        
        public Shop(string shopName, string locationId, int id) 
        {
            setShopName(shopName);
            setLocationId(locationId);
            setId(id);
            IsOpen = setIsOpen();
        }
        
        public void setShopName(string shopName) 
        {
            if (string.IsNullOrWhiteSpace(shopName)) 
            {
                throw new ArgumentException($"{shopName} is null or white spaces.");
            }

            ShopName = shopName;
        }

        public void setLocationId(string locationId)
        {
            if (string.IsNullOrWhiteSpace(locationId))
            {
                throw new ArgumentException(nameof(locationId));
            }
            else if (locationId.Length != 3) 
            {
                throw new ArgumentException(nameof(locationId));
            }

            LocationId = locationId;
        }
        
        public void setId(int id)
        {
            if (id <= 0)
            {
                throw new ArgumentOutOfRangeException($"{id} is lower than 0");
            }

            Id = id;
        }

        //method sets the property to true when hour is between 7 and 21.
        //the shop is closed on Sundays then return false when day of week is 0 (Sunday) 
        private bool setIsOpen() 
        {
            if((int)DateTime.Now.DayOfWeek == 0) 
            {
                return false;
            }

            if (!(DateTime.Now.Hour >= 7 && DateTime.Now.Hour <=21)) 
            {
                return false;
            }

            return true;
        }

        public override string ToString()
        {
            string ShopInformation = "";
            if (IsOpen == true) 
            {
                ShopInformation = "ShopName: " + "\nLocationId: " + LocationId + "\nId: " + Id + "\nIsOpen: Yes"; 
            }
            else 
            {
                ShopInformation = "ShopName: " + "\nLocationId: " + LocationId + "\nId: " + Id + "\nIsOpen: No";
            }
            return ShopInformation;
        }
    }
}
3

Po co ci te metody set, skoro masz właściwości? Rzucać wyjątkami jeszcze można przy tworzeniu obiektu, ale już później to tak chyba średnio. Metodę tostring możesz uprościć do jednej linijki przy użyciu : https://docs.microsoft.com/pl-pl/dotnet/csharp/language-reference/operators/conditional-operator

3
  1. W C# nazwy metod powinny się zaczynać od dużej litery.
  2. Twoja metoda setIsOpen niczego nie ustawia tylko zwraca wynik boolean. Zamiast tego powinieneś po prostu użyć read only właściwości IsOpen i tak to obliczać. Ewentualnie wystawić publiczna metodę IsOpen zwracającą bool.
  3. Ogólnie to tak jak napisał @kzkzg większość z tego możesz zamienić na właściwości i usunąć te metody.
0

@Aventus: Czyli lepiej byłoby to zrobić w taki sposób?

private string _shopName;

public string ShopName 
        {
            get => _shopName;
            set 
            {
                if (string.IsNullOrWhiteSpace(value))
                {
                    throw new ArgumentException($"{value} is null or white spaces.");
                }
                _shopName = value;
            }
        }
2

W takich prostych przypadkach tak.

0

@Aventus: A co gdyby walidacja obejmowała dużo rzeczy do sprawdzenia? Przy większej ilości kodu ma sens tworzenie metody SetShopName(), która to wszystko posprawdza dla tego pola, tak jak zrobiłem to na początku?
A no i jeszcze co z ewentualną możliwością korekty danych dla użytkownika? Pisać w tej samej klasie metody które pozwolą użytkownikowi podać jeszcze raz nazwę sklepu, czy może oddzielna klasa, która będzie za takie coś odpowiadała? Z tego co rozumiem klasa powinna być odpowiedzialna za jedną rzecz, więc skłaniałbym się wydzielić inną klasę pozwalającą na korektę i ewentualnie wykorzystać ją w set później. Pytanie czy taki tok rozumowania jest prawidłowy?

4
Abachaczi napisał(a):

@Aventus: A co gdyby walidacja obejmowała dużo rzeczy do sprawdzenia? Przy większej ilości kodu ma sens tworzenie metody SetShopName(), która to wszystko posprawdza dla tego pola, tak jak zrobiłem to na początku?

A no i jeszcze co z ewentualną możliwością korekty danych dla użytkownika? Pisać w tej samej klasie metody które pozwolą użytkownikowi podać jeszcze raz nazwę sklepu, czy może oddzielna klasa, która będzie za takie coś odpowiadała? Z tego co rozumiem klasa powinna być odpowiedzialna za jedną rzecz, więc skłaniałbym się wydzielić inną klasę pozwalającą na korektę i ewentualnie wykorzystać ją w set później. Pytanie czy taki tok rozumowania jest prawidłowy?

Wtedy najlepiej wynieść walidację do osobnej klasy. Można zacząć od zrobienia wszystkich właściwości z automatycznymi getterami i setterami.

public class Shop
{
    public string ShopName { get; set; }
    public string LocationId { get; set; }
    public int Id { get; set; }
    public bool IsOpen { get; set; }
}

Interfejs dla walidatora oraz klasa błędu walidacji:

public class ValidationError
{
    public ValidationError(string propertyName, string errorMessage)
    {
        PropertyName = propertyName;
        ErrorMessage = errorMessage;
    }

    public string PropertyName { get; }
    public string ErrorMessage { get; }
}

public interface IValidator<in T>
{
    IReadOnlyList<ValidationError> Validate(T model);
}

Oraz walidator dla klasy Shop:

public class ShopValidator : IValidator<Shop>
{
    public IReadOnlyList<ValidationError> Validate(Shop model)
    {
        var result = new List<ValidationError>();
        
        if (string.IsNullOrWhiteSpace(model.ShopName)) 
        {
            result.Add(new ValidationError(nameof(model.ShopName), "shop name cannot be null"));
        }
        
        // ...
        // other validations
        
        return result;
    }
}

Na szczęście żeby nie robić tego wszystkiego samemu ręcznie można wykorzystać jakąś gotową bibliotekę do walidacji. Polecam hasło w google validation library c#. Najpopularniejsza jest FluentValidation i ja jej najczęściej używam.

3

Są różne patterny do tworzenia obiektów np. Builder, wtedy taką klasę udostępniasz jedynie poprzez Builder (prywatne konstruktory), a Builder zapewnia że dane są poprawne.

var buildingResult = new ShopBuilder()
					 .SetName("xd")
					 .SetLocation("fg")
				 	 .TryBuild();

if (!buildingResult.Succeed)
	...
2

Co do takich prostych walidacji to tak jak napisali koledzy wyżej. Jeśli chodzi o egzekwowanie zasad biznesowych to ja jestem za tym żeby trzymać to najbliżej źródła, czyli w samych obiektach domenowych/biznesowych. W przeciwnym razie obiekty te stają się tylko zwykłymi pojemnikami na dane, co samo w sobie jest przeciwne do tego czym OOP miało być w założeniu. Obiekty powinny posiadać dane, ale i zachowanie (ang. behaviour).

0

W sumie zastanawiam się jeszcze czy na początku nauki, we w miarę prostych aplikacjach konsolowych jest sens implementować bardziej złożone rzeczy. Chyba zrobię to w miarę jak najprościej się da na tym etapie aby potestować rzeczy których się uczę z językaa gdy przejdę do nauki ASP.NET Core zacznę tego się uczyć naprawdę mocno i przyłożę do tego większą uwagę. Ciężko w tych aplikacjach konsolowych jakoś implementować takie bardziej złożone mechanizmy, wydaje mi się, że tworząc np. Stronę internetową, czy jakiś backend do systemu logowania, sprzedaży biletów będzie łatwiej to odwzorować wszystko :D

1

@Abachaczi:

Aplikacja webowa w ASP .NET Core to też konsolka, Winform też pod spodem ma konsolkę - różnica głównie polega na templatce projektu :)

Ogranicza cię jedynie pomysł :p

3
Abachaczi napisał(a):

wydaje mi się, że tworząc np. Stronę internetową, czy jakiś backend do systemu logowania, sprzedaży biletów będzie łatwiej to odwzorować wszystko :D

Zrób taki backend do sprzedaży biletów w konsoli, jeśli potem dorobienie do tego warstwy webowej nie będzie męczarnią, to znaczy, że zrobiłeś dobrze.
Co do głównego pytania, to:

  1. Obiekt reprezentujący Twój obiekt biznesowy (encja) powinna być tworzona przez konstruktor, i tam rzucać wyjątkami, jeśli dane są niepoprawne.
  2. Dane dostajesz w jakiś sposób od użytkownika (czy ogólniej rzecz biorąc, z zewnątrz). Takie dane powinieneś zwalidować przy użyciu odrębnej klasy/klas, i zwrócić informację użytkownikowi (na zewnątrz), na temat tego, co było błędne, aby mógł poprawić. I to wszystko bez tworzenia Twojego biznesowego obiektu.
  3. Budowa obiektów może być czasami dość skomplikowana - nawet mając prawidłowe dane, można chcieć zbudować obiekt w jakiś konkretny sposób, albo np. obiekty mogą mieć różne dane opcjonalne, wówczas przydają się jeszcze inne klasy pomocnicze do tworzenia docelowych obiektów, np. buildery czy fabryki. One też dodatkowo mogą sprawdzać poprawność danych do nich przekazanych.
0

@somekind:

  1. Dane dostajesz w jakiś sposób od użytkownika (czy ogólniej rzecz biorąc, z zewnątrz). Takie dane powinieneś zwalidować przy użyciu odrębnej klasy/klas, i zwrócić informację użytkownikowi (na zewnątrz), na temat tego, co było błędne, aby mógł poprawić. I to wszystko bez tworzenia Twojego biznesowego obiektu.

Czyli Np. w przypadku pobierania danych od użytkownika mogę mieć klasę ReadDataFromUser i w niej ewentualnie sprawdzam poprawność danych, daję możliwość poprawy i już z poprawnymi danymi tworzę instancję klasy? (Chyba, że ewentualnie ReadDataFromUser zajmie się tylko wczytaniem danych a sprawdzeniem jeszcze inna tylko walidująca dane)

  1. Obiekt reprezentujący Twój obiekt biznesowy (encja) powinna być tworzona przez konstruktor, i tam rzucać wyjątkami, jeśli dane są niepoprawne.

Na ten moment mam to zrobione w skrócie tak (podaje przykład z jednym polem, inne wyglądają podobnie):

 public class Shop
    {
        public Shop(string shopName)
        {
            ShopName = shopName;
           
        }

        private string _shopName;

        public string ShopName 
        {
            get => _shopName;
            set 
            {
                if (string.IsNullOrWhiteSpace(value))
                {
                    throw new ArgumentException($"{value} is null or white spaces.");
                }
                _shopName = value;
            }
        }

No i w sumie podczas przypisywania wartości w konstruktorze rzuci wyjątek bo set będzie odpowiedzialny za sprawdzenie i ustawienie. Czy o to mniej więcej chodzi w tym punkcie co napisałeś, że jak już to powinien w konstruktorze ten wyjątek rzucić, podczas tworzenia encji? Tak jak wyżej pisałem chyba zwaliduję dane w innej klasie np. czytającej od użytkownika dane.

1

ja bym napisał klasę która nie pozwala na modyfikowanie pól po tym jak został obiekt takiej klasy stworzony. (czyli to o czym pisal @somekind )

Obiekt reprezentujący Twój obiekt biznesowy (encja) powinna być tworzona przez konstruktor, i tam rzucać wyjątkami, jeśli dane są niepoprawne.

public class Shop
{
        public string ShopName { get;  } // nie trzeba definiowac private set, bez definicji set jest prywatny
        public string LocationId { get;  }
        public int Id { get; }
        //public bool IsOpen { get; } nie jest potrzebne Ci to pole, mozesz zwrocic po prostu wynik operacji

        public Shop(string shopName, string locationId, int id = int.MinValue) 
        {
            if (string.IsNullOrWhiteSpace(shopName)) 
            {
                throw new ArgumentException(nameof(model.ShopName), "shop name cannot be null"));
            }
           if (string.IsNullOrWhiteSpace(locationId)) 
           {
                throw new ArgumentException(nameof(model.ShopName), "locationId name cannot be null"));
           }
           if (id == int.MinValue) 
           {
                throw new ArgumentException(nameof(model.ShopName), "id cannot be null"));
           }
          ShopName = shopName;
          LocationId = locationId;
          Id = id;
        }

        public bool IsOpen() 
        {
            if((int)DateTime.Now.DayOfWeek == 0) 
            {
                return false;
            }

            if (!(DateTime.Now.Hour >= 7 && DateTime.Now.Hour <=21)) 
            {
                return false;
            }

            return true;
        }
}
0
fasadin napisał(a):

ja bym napisał klasę która nie pozwala na modyfikowanie pól po tym jak został obiekt takiej klasy stworzony. (czyli to o czym pisal @somekind )

Obiekt reprezentujący Twój obiekt biznesowy (encja) powinna być tworzona przez konstruktor, i tam rzucać wyjątkami, jeśli dane są niepoprawne.

public class Shop
{
        public string ShopName { get;  } // nie trzeba definiowac private set, bez definicji set jest prywatny

Ale bez private/protected set nie da rady przypisać wartośći w innym miejscu niż konstruktor (readonly). Więc {get; private set;} to nie to samo co {get;}

2

@szydlak: no i to chodzi żeby pola się nie zmieniały, czyli żeby był to obiekt niemutowalny.

2
Aleksander32 napisał(a):

@szydlak: no i to chodzi żeby pola się nie zmieniały, czyli żeby był to obiekt niemutowalny.

Ale ja to rozumiem. Tylko stwierdzenie, że:

nie trzeba definiowac private set, bez definicji set jest prywatny

nie do końca jest prawdą.

4
Abachaczi napisał(a):

Czyli Np. w przypadku pobierania danych od użytkownika mogę mieć klasę ReadDataFromUser i w niej ewentualnie sprawdzam poprawność danych, daję możliwość poprawy i już z poprawnymi danymi tworzę instancję klasy?

W ogólności tak, chociaż ReadDataFromUser to bardziej nazwa metody niż klasy.

W konsoli jesteś w tej komfortowej sytuacji, że możesz zamknąć użytkownika w nieskończonej pętli, aż poda dane prawidłowo. W przypadku jakiegoś API internetowego albo czytania danych z pliku jest o tyle trudniej, że można jedynie zwracać błędy.

(Chyba, że ewentualnie ReadDataFromUser zajmie się tylko wczytaniem danych a sprawdzeniem jeszcze inna tylko walidująca dane)

To jest zdecydowanie bardziej przyszłościowe rozwiązanie biorąc pod uwagę to, co napisałem wyżej. W webowym API klasy walidującej dane będziesz mógł użyć, aby błąd z niej zwracany przekształcić na błąd HTTP i odesłać do użytkownika. W konsoli możesz za to taki błąd obsłużyć inaczej, pytając użytkownika o dane do skutku. Jeśli będziesz miał oddzielną logikę walidacji, to będziesz mógł użyć jej w obu aplikacjach bez żadnych zmian. Jeśli to połączysz, to pracując nad webowym API będziesz musiał zaimplementować taką samą walidację ponownie.

No i w sumie podczas przypisywania wartości w konstruktorze rzuci wyjątek bo set będzie odpowiedzialny za sprawdzenie i ustawienie. Czy o to mniej więcej chodzi w tym punkcie co napisałeś, że jak już to powinien w konstruktorze ten wyjątek rzucić, podczas tworzenia encji? Tak jak wyżej pisałem chyba zwaliduję dane w innej klasie np. czytającej od użytkownika dane.

A po co ten publiczny seter? Jaki jest sens, aby obiekt mógł zmieniać swoją nazwę po utworzeniu?
Jeśli nie ma takiej potrzeby, to i publiczny seter jest niepotrzebny. A w takiej sytuacji wystarczy rzucić wyjątek w konstruktorze, mniej więcej tak, jak to @fasadin pokazał wyżej.
Lepiej mieć mniej miejsc, gdzie coś może się zmienić niż więcej, bo im więcej tym tym trudniej później nad tym zapanować.

@fasadin, teraz zamiast tak:

 if (string.IsNullOrWhiteSpace(shopName)) 
{
      throw new ArgumentException(nameof(model.ShopName), "shop name cannot be null"));
}
ShopName = shopName;

Można tak:

ShopName = shopName ?? throw new ArgumentNullException(nameof(shopName));
0

Dobra myślę, że teraz wiele rzeczy mi się rozjaśniło. Właśnie głównie szukałem odpowiedzi na pytanie jak to się robi później np. w aplikacjach webowych. Z pisaniem klas to niby podstawy podstaw, wszystko takie proste w poradnikach ale jednak można nabrać złych nawyków albo później trzeba przepisywać wszystko od nowa (chociaż to pewnie nie raz się zdarzy jeszcze xd). Pytanie wydawało mi się aż za głupie żeby pytać na forum bo niby prosta rzecz ale jednak warto pytać nawet o podstawy :D W takiej prostej logice którą mam ograniczę się do jakiejś prostej klasy czytającej od użytkownika i ewentualnie pokombinuję sobie z walidacją oddzielną klasą, będzie bardziej przyszłościowo jak radził @somekind

0
somekind napisał(a):
Abachaczi napisał(a):

wydaje mi się, że tworząc np. Stronę internetową, czy jakiś backend do systemu logowania, sprzedaży biletów będzie łatwiej to odwzorować wszystko :D

Zrób taki backend do sprzedaży biletów w konsoli, jeśli potem dorobienie do tego warstwy webowej nie będzie męczarnią, to znaczy, że zrobiłeś dobrze.

A to bardzo ciekawy projekt konsolowy może być :) aż chętnie sam go wykonam. Jeśli chodzi o rezerwacje to będziemy musieli skorzystać z bazy danych (Entity framework chyba jest tutaj najpopularniejszy?). Dodatkowo kilka osób na raz może otworzyć konsole i spróbować dokonać rezerwacji czyli jakieś aspekty programowania wielowątkowego wprowadzić? Czyli trzeba by zastosować "lock" ?

0

@kzkzg: To wszystko zależy od tego, czy twoja klasa będzie używana wyłącznie w twoim programie czy w różnych programach tak jak biblioteki NET .
Jeśli w różnych programach to każdy błąd powinien być jakoś sygnalizowany na zewnątrz.
Ta klasa którą wymyślił Abachaczi jest trochę dziwaczna ale on pisał że jest początkujący więc można mu wybaczyć. ;)

0
gornada napisał(a):

Jeśli chodzi o rezerwacje to będziemy musieli skorzystać z bazy danych (Entity framework chyba jest tutaj najpopularniejszy?).

Pewnie jest najpopularniejszy, ale nie jest bazą danych.
Czy potrzebujesz aż relacyjnej bazy danych do takiego projektu, to nie wiem. Na początek dane można równie dobrze trzymać w plikach.

Dodatkowo kilka osób na raz może otworzyć konsole i spróbować dokonać rezerwacji czyli jakieś aspekty programowania wielowątkowego wprowadzić? Czyli trzeba by zastosować "lock" ?

No jeśli źródło danych będzie w pamięci operacyjnej, to może i lock. Jeśli będzie nim baza danych, to wystarczy sprawdzić, czy podany termin nie został jeszcze zarezerwowany.

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