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

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

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);}
    }
}

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. Aż się prosi o przeładowanie ToString() zamiast tworzyć propertę ‘FullName’.

2. To byłoby brzydkie i nieintuicyjne oraz powodowałoby problemy np. przy bindingu.

3. 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…

4. 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.

5. 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 😉

6. Wszystko ok, tylko po co ten region

7. http://msdn.microsoft.com/en-us/library/9a1ybwek%28v=vs.71%29.aspx

8. 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)

9. 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.

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

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

11. 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; }

12. 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?

13. 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.

14. 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:)

15. @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ę.