Code Review: pola w C#

Co byście powiedzieli na taki kod?

public class Person
{
   public string FirstName;
   public string  LastName;

   public string GetFullName()
   {
       return string.Format("{0} {1}", FirstName, LastName);
   }       
}

Na pierwszy rzut oka może nic poważnego. Jednak jeśli chcemy pisać kod zgodny z praktykami C#, powinniśmy zwrócić uwagę na następujące kwestie:

  • Enkapsulacja, FirstName,LastName powinni być ukryte i ewentualnie wyeksponowane za pomocą setterow\getterów.
  • GetFullName jest dobrym sposobem dla Javy (a w zasadzie getFullName). W C# do tego celu używamy właściwości.

Po refaktoryzacji otrzymujemy:

public class Person
{
    private string _firstName;
    public string FirstName
    {
        get{return _firstName;}
        set{ _firstName = value;}
    }
    private string _lastName;
    public string LastName
    {
        get{return _lastName;}
        set{ _lastName = value;}
    }

    public string FullName
    {
        get{ return string.Format("{0} {1}",_firstName, _lastName);}
    }
}

Kod wygląda dużo lepiej. Osobiście jednak dokonałbym dwóch modyfikacji:

  1. Jeśli nie mamy żadnej logiki (typu wywołanie zdarzenia) w setterach, lepiej użyć automatic properties.
  2. “Owinąć” w regiony kod.

Po kolejnej refaktoryzacji otrzymujemy:

public class Person
{
   #region Properties

   public string FirstName { get; set; }
   public string LastName { get; set; }
   public string FullName
   {
       get { return string.Format("{0} {1}", FirstName, LastName); }
   }

   #endregion
}

15 thoughts on “Code Review: pola w C#”

  1. Jak byśmy chcieli zrobić kod bulletproof to Full Name powinno korzystać z ustawień regionalnych, ponieważ “gdzieś w głębokiej puszczy” Full Name może oznaczać nazwisko imie, a nawet jeśli wszędzie na świecie przyjmuję się imię nazwisko to pytanie czy odstęp w postaci spacji jest standardem w wszystkich krajach…

  2. Ale po co sie o to martwic? Nie piszemy kodu uniwersalnego na wszystko – piszemy go w zaleznosci od wymagan.
    Podejscie pisania kodu maksymalnego elastycznego (jak to tylko mozliwe) jest bardzo zle.

  3. spoko, dobry post, dobra seria, chcialbym zeby wczesniej powstała bo teraz to juz niektore odcinki ;)) sa oczywiste dla mnie. Ale sporo osob na pewno skorzysta.
    Kontynuuj waćpan 😉

  4. Prawdę mówiąc spodziewałem się że będziesz trochę niepoprawny politycznie i powiesz kiedy pola sprawiają mniej problemów niż właściwości (dyrektywy ref i out parametrów metod, oraz jakieś super optymalizacje wydajnościowe) . Metoda GetName (albo lepiej RetrieveName) sugeruje że wartość jest wyliczana w momencie wywołania, natomiast użycie gettera sprawia że osoba korzystająca z niego uważa że wartość jest już policzona albo policzy się szybko.
    Przeciążanie metody ToString uważam za przydatne przy Debugu (nie trzeba rozwijać 1500 pól i właściwości w quickwatchu żeby dowiedzieć się co to za obiekt tylko wystarczy jeden rzut okiem), należy to jednak robić z głową (id i nazwa klienta sprawdza się lepiej niż adres i imie psa oraz teściowej)

  5. Osobiscie przy takiej ilosci kodu odpuscilbym sobie #region, a ToString() przeciazyl tylko w celu Debugowania (pozniej usunal).
    Sam sens FullName zalezy od wymagan/ustalen. Prawie zawsze jest to kod zwiazany typowo z wizualna prezentacja danych i powinien sie znajdować w warstwie prezentacji zeby nie zasmiecac obiektow biznesowych. Samo formatowanie wartosci mozemy zalatwic przy uzyciu AutoMappera lub jakiegos formattera (konfigurowalnosc).
    @Wojtek – zgadzam sie w kwestii obliczania wartosci. Malo kiedy zmieniamy jedna wlasciwosc tego typu obiektu i odrazu potrzebujemy jej specjalnie sformatowanej, wiec z punktu widzenia wydajnosci kazdorazowe formatowanie jest potencjalnym marnotrawstwem zasobow.

  6. raczej kiepski argument zwłaszcza, że jest DebuggerDisplay 😉

    [DebuggerDisplay(“Name = {FirstName} LastName = {LastName}”)]

  7. Swoją drogą, osobiście jeżeli pole ma być dostępne do zapisu/odczytu na tym samym poziomie to osobiście odpuszczam sobie {get; set; }

  8. 1. Co to ToString to do tego sluzy jak wspomnial nilphilus DebuggerDisplay. Mozna przeladowac ToString ale dodatkowo a nie usuwajac FullName. Powiedzialbym nawet, ze przeladowanie ToString w celach debbugowych jest kolejnym bad-practice:). Nie powinnismy modyfikwoac kodu aby ulatwic sobie debuggowanie. Do tego sluza atrybuty, ktore nie zmianiaja zachowania klasy.
    2. Co do wydajnosci. To nie jest marnotractwo. Mozesz zrobic lazy loading i cachowac rezultat za kazdym razem gdy FirstName albo LastName sie zmienil. Ale gwarantuje Ci, ze ten mechanizm wiecej problemow i niepotrzebnej zawilosci wprowadzi.
    3. Wlasciwosci nie sugeruja, ze sa to zwykle zmienne. Dopuszczalna jest logika we wlasciwosciach jesli jest ona PROSTA. Np. IsConnected moze sprawdzac wewnetrzny stan. Gdy jest skomplikowana przenosimy to do FormatName czy cos takeigo.
    4. Region – doswiadczenie mi mowi, ze wczesniej czy pozniej nalezy wprowadzic regiony bo pojawiaja sie metody, zdarzenia itp.

    Ktos wspomnial o warstwie prezentacji itp. Ale moze jest to prostu ViewModel? I wtedy ma to sens aby wyeksponowac FullName wlasnie za pomoca wlasciwosci? Mozna oczywiscie skorzystac rowniez z Converter’a jesli robimy w WPF.
    BTW. Co ma AutoMapper do tego?

  9. 1. true, na dluzsza mete to jest to zle
    2. true, nalezy kierowac sie rozsadkiem
    3. true
    4. true, raczej pozniej =;)

    Wlasnie dla ViewModela potrzebujemy (zazwyczaj) jednokierunkowej konwersji/formatowania danych, a AutoMapper ma tyle do tego: Mapper.CreateMap().ForMember(d => d.FullName, o => o.MapFrom(s => string.Format(“{0} {1}”, s.FirstName, s.LastName))) – jednorazowa operacja.

  10. Nie jest zbyt to czytelne. W momencie gdy zmienia sie FirstName lub LastName chcemy odswierzyc FullName – wywolujemy OnPropetyChanged i WPF pobiera dane. Tylko raz. Jest to duzo bardziej czytelne. A wlasciwosc przeciez i tak potrzebujemy dla ViewModel. Jak w przypadku AutoMappera podbindowalbys to do XAML?

    Inny sposob to Converter i akurat w przypdku WPF jest to najlepsza opcja:)

  11. @Wojciech(szogun1987)
    cytując: “Metoda GetName (albo lepiej RetrieveName) sugeruje że wartość jest wyliczana w momencie wywołania, natomiast użycie gettera sprawia że osoba korzystająca z niego uważa że wartość jest już policzona albo policzy się szybko.”
    Masz rację i to podejście jest niezwykle istotne np. dla IList, tzn.

    a) public IList GetItem()
    b) public IList Items

    Ta notacja dodatkowo sugeruje że w a) produkowana jest lista a w b) że jest to ta sama referencja, ma to szczególne znaczenia gdy chcemy np. dodawać elementy do danej kolekcji. Nie powinno się zatem nigdy używać przypadku b) gdy produkujemy listę.

Leave a Reply

Your email address will not be published.