Czy jest to gówno kod?

0

Cześć wszystkim, bardzo potrzebuje waszej oceny, czy jest ten kod "g**no kod"? Czy prawidłowo skorzystałem z kompozycji, hermetyzacji(Inkapsulacji) i dziedziczenia? Jakie są błedy i co lepiej zmienić?

Jest to nie wielka praktyka, tworzenie unique obiektu, dziedzicząc classę Person i dodając kompozycję Job.

Dziękuję bardzo.

package composition;

public class Composition {
    public static void main(String[] args) {
        Vladyslav vladyslav = Vladyslav.getInstance();  
        vladyslav.setName("Vladyslav");
        vladyslav.setSecond_name("Parkhomenko");
        vladyslav.setGender("Male");
        vladyslav.setAge(18);
        
        System.out.println("\n" + vladyslav.getName() + ", " + vladyslav.getSecond_name() + ", " + vladyslav.gender + ", " + vladyslav.getAge() + "years.");
        vladyslav.showJobInfo();
        System.out.println(vladyslav.getClass());
    }
    
}

package composition;

public class Person {
    protected String name;
    protected String second_name;
    protected String gender;
    protected int age;
    
    public void setName(String name){
        this.name = name;
    }
    public String getName(){
        return name;
    }
    
    public void setSecond_name(String second_name){
        this.second_name = second_name;
    }
    public String getSecond_name(){
        return second_name;
    }
    
    public void setGender(String gender){
        this.gender = gender;
    }
    public String getGender(){
        return gender;
    }
    
    public void setAge(int age){
        this.age = age;
    }
    public int getAge(){
        return age;
    }
}
package composition;

public class Job {
    protected String titleOfJob;
    protected int salaryInApples;
    protected int businessID;
}
package composition;

public class Vladyslav extends Person{
    private static Vladyslav instance;
    private Vladyslav(){}
    
    public static Vladyslav getInstance(){
        if(instance == null) {
            instance = new Vladyslav();
            System.out.println("Unique Vladyslav was created.");
        }
        else
            System.out.println("Vladyslav had created yet.");
        return instance;
    }


    void showJobInfo(){
        Job job = new Job();
        job.titleOfJob = "Student";
        job.salaryInApples = 250;
        job.businessID = 96822842;
        System.out.println("Status in community is " + job.titleOfJob + ", he earns " + job.salaryInApples + "apples per month and BusinessID is " + job.businessID);
    }
}

1

Vladyslav nie powinien być klasą a instancją klasy Person.
ShowJobInfo nie powinno tworzyć nowej pracy a wyświetlać już istniejącą. Innymi słowy powinieneś przechowywać instancję klasy Job. Może być w klasie Person.
Jak dotąd kompozycji w twym kodzie nie widzę.
Inkapsulacja? Ja znam jedynie enkapsulację. Tą wykonałeś poprawnie, ale i tak oczy od tego bolą. Nie ma Java właściwości może?
EDIT:
Przydałby ci się konstruktor który przyjmuje właściwości takie jak imię, nazwisko itp jako parametry.
EDIT2:
W Javie się pisze snake_casem? Przyznasz chyba, że to wygląda dziwacznie: getSecond_name

5
  1. Dramat.
        vladyslav.setName("Vladyslav");
        vladyslav.setSecond_name("Parkhomenko");
        vladyslav.setGender("Male");
        vladyslav.setAge(18);

Nie widzę tu tej twojej enkapsulacji ani trochę. Przecież żywcem dobierasz się do detali jakiegoś obiektu! Jakbyś pisał symulator jazdy samochodem to zrobisz

car.startEngine();
car.drive();

czy

car.getWheels().getFrontLeft().turnALittle();
car.getWheels().getFrontRight().turnALittle();
...

W obiektach interesuje cię ich kontrakt, interfejs, biznesowy sposób użycia, a nie detale implementacyjne. Zamiast jakiegoś vladyslav.getGender().equals("Male") powinno być np. vladyslav.isMale() bez wchodzenia w szczegóły jak to jest pod spodem zrobione.

  1. Nie rozumiem czemu Vladyslav jest szczególnym TYPEM osoby.
  2. showJobInfo jest bez sensu, bo tworzysz ten obiekt w chwili wywołania za każdym razem, czemu nie stworzysz go raz i nie przypiszesz do pola? Przecież jak chcesz jechać samochodem to nie jest tak że przykręcasz koła, trochę podjeżdżasz a potem odkręcasz koła (a to właśnie teraz robisz w tej metodzie :D)
0

Zrób zamiast setterów builder pattern. Będzie odrobinę mniej brzydko.

// pseudo-kod
Person vladyslav = new Person.Builder().withName("Vladyslav").withSecondName("Parkhomenko").withAge(18).withGender(Person.Gender.Male).build();
vladyslav.showJobInfo();

Hasło do Googla: "java builder pattern".

Edit: BTW, klasa "Vladyslav" miała by sens gdyby miała metodę "clone" :-D (taki niszowy żarcik)

6

Czy prawidłowo skorzystałem z kompozycji, hermetyzacji(Inkapsulacji) i dziedziczenia? Jakie są błedy i co lepiej zmienić?

Tu myślę jest błąd w myśleniu. Skupianie się na "korzystaniu z kompozycji, hermetyzacji i dziedziczenia", zamiast na rozwiązywaniu realnych problemów.

Jest to nie wielka praktyka, tworzenie unique obiektu, dziedzicząc classę Person i dodając kompozycję Job.

Podstawowe pytanie: po co ci to?

Czy jest to g**no kod?

Pewnie tak, ale nawet nie chodzi o to czy jest ładny czy brzydki, tylko, że to sprawia wrażenie kodu szkoleniowego, czyli z definicji będzie to g**no kod, ponieważ nic to nie robi, a jedynie zaciemniasz sytuację. Ten cały kod można zmieścić w 2 linjkach pseudokodu:

Vladyslav, Parkhomenko, Male, 18
Status in community is Student, he earns 250 apples per month and BusinessID is 96822842

bo to jest esencja tej apki, te dane i co wyświetla.
robiąc z tego całą "aplikację" w 4 plikach, popełniasz po prostu gruby overengineering.

Chyba, że ten kod ma czemuś więcej służyć (np. obsługiwać ileś ludzi naraz, dodać możliwość wyszukiwania, synchronizować z jakąś bazą itp.), innymi słowy jeśli ma jakąś wartość użyteczną. Ale tego nie napisałeś. Inaczej to tylko zabawa w "jak obrobić 2 wiersze danych w najbardziej skomplikowany sposób, żeby wyglądało to jak aplikacja".

Problem tylko, że ciężko cokolwiek powiedzieć o architekturze, dobrych praktykach, skoro to apka, która niczemu nie służy konkretnemu, a to, czy zrobić taką klasę czy inną, na to też nie da się odpowiedzieć (bo na tym etapie samo robienie klasy jest overengineeringiem, skoro chodzi tylko o trzymanie danych i wyświetlenia prostego napisu.

Powiem tak - więcej się nauczysz robiąc coś realnego, co nawet będzie napisane spaghetti kodem, ale potem wyciągając wnioski "aha, złamałem enkapsulację i taka tego konsekwencja była (np. złamanie enkapsulacji może spowodować trudności z łatwym wprowadzaniem zmian w kodzie aplikacji na poźniejszych etapach)", niż zastanawianie się nad idealnym zapisem na sucho klasy, która nic nie robi praktycznie, poza tym, że jest przykładem ala książka...

1

To jest gównokod, bo stosujesz wzorce projektowe na siłę. Dziedziczenie z Persons zupełnie bez sensu, a i sama klasa też trochę na siłę. Ja w ogóle zamiast bawić się w tyle pól, zrobiłbym tam w środku słownik i ewentualnie walidował czy są w nim wymagane klucze i ich typy. Byłoby elastyczniej i zaraz docenisz jak będziesz chciał to serializować i deserializować. W tym metoda toString() też będzie o niebo prostsza (i ona powinna być zamiast stertą geterów w :rintfie poza klasą). Z Show Job info, podobnie: System.out.println(person.getJob().toString());. No chyba że chcesz pisać jakiś konsolowy CRUD (sic!)

1
Shalom napisał(a):
  1. Dramat.
        vladyslav.setName("Vladyslav");
        vladyslav.setSecond_name("Parkhomenko");
        vladyslav.setGender("Male");
        vladyslav.setAge(18);

Nie widzę tu tej twojej enkapsulacji ani trochę. Przecież żywcem dobierasz się do detali jakiegoś obiektu!

Jest enkapsulacja o tyle, że pola są protected, a dostęp do nich jest poprzez settery i gettery. To najprostsza forma enkapsulacji.

Jakbyś pisał symulator jazdy samochodem to zrobisz

car.startEngine();
car.drive();

czy

car.getWheels().getFrontLeft().turnALittle();
car.getWheels().getFrontRight().turnALittle();
...

Ten przykład nie jest trafiony, bo ilustruje złamanie prawa Demeter. Jego kod pozostawia wiele do życzenia - np. ja życzyłbym sobie go nie obejrzeć - ale akurat tego nie popełnił, zapewne przeoczywszy, że można. Metody wywołuje bezpośrednio na obiekcie Person. Nie wiemy, czy setAge zmienia stan obiektu Person, czy może obiekt Person deleguje go pod spodem do jakiegoś innego obiektu, itp.

3

Jest enkapsulacja o tyle, że pola są protected, a dostęp do nich jest poprzez settery i gettery.

To nie jest enkapsulacja tylko robienie prostytutki z logiki. Jak zrobisz pola private ale dasz getter i setter to równie dobrze mogleś je zrobić public. I tu i tu wyciągasz na zewnątrz wewnętrzny stan obiektu.

4

skorzystałem z kompozycji, hermetyzacji(Inkapsulacji) i dziedziczenia? - tu jest główny błąd.
Chciałeś na siłę skorzystać z jakiś koncepcji, a nie wymyśliłeś po co.
Wymyśl najpier co program ma robić - może być proste, a potem pisz rozwiązanie, korzystając z jakichś narzędzi (o ile się przydadzą).
Inaczej tworzy się potwory i można (oj można...) się tak nauczyć pisać kod: bierzemy nowy młotek z półki i szukamy w co by tu przywalić.
https://github.com/EnterpriseQualityCoding/FizzBuzzEnterpriseEdition

1

Przede wszystkim nie wiem co wspólnego mają DTOsy z programowaniem obiektowym... tylko że to nawet DTOs porządny nie był :D
Jak słusznie @LukeJL zauważył to jest przykład szkolny czyli do d**y. Przykładem enkapsulacji to może być klasa ThreadPoolExecutor czy RestTemplate a nie biedny DTOs

0

lombok pozwala na zmniejszenie ilosci getterow i setterow, zwiekszenie tym samym czytelnosci kodu

4
Shalom napisał(a):

Jest enkapsulacja o tyle, że pola są protected, a dostęp do nich jest poprzez settery i gettery.

To nie jest enkapsulacja tylko robienie prostytutki z logiki. Jak zrobisz pola private ale dasz getter i setter to równie dobrze mogleś je zrobić public. I tu i tu wyciągasz na zewnątrz wewnętrzny stan obiektu.

W obecnej postaci tak akurat jest. Z punktu widzenia enkapsulacji zasadnicze jest to, że mając metody dostępowe możesz zmienić tę implementację nie zmieniając API obiektu. Kod wywołujący NIE WIE, i nie musi wiedzieć, co "pod spodem" robi ten setter i getter. Bez zajrzenia w kod nie wiedziałbyś, że tam są pola private, prawda?

Inymi słowy kod wywołujący jest odporny na zmiany tej implementacji - np. na zastąpienie jej przez mniej naiwną. A w przypadku odwołania do pola publicznego bezpośrednio już by tak nie było. Gdybyś np. chciał dodać jakąś logikę walidacyjną, to w takim wypadku musiałbyś zmienić API obiektu. A tu nie musisz. I to jest enkapsulacja.

1

W obecnej postaci tak akurat jest. Z punktu widzenia enkapsulacji zasadnicze jest to, że mając metody dostępowe możesz zmienić tę implementację nie zmieniając API obiektu. K

Z jednej strony tak i na tym przykładzie pewnie to trafny kontrargument, ale jednak na nieco innym przykładzie już nie musi być tak kolorowo.

Dając settery, zakładamy, że:

  • obiekt w ogóle ma pewne właściwości o określonym typie (a jeśli w przyszłości będziemy chcieli całkowicie zaorać struktury danych w danej klasie?)
  • właściwości da się uzyskać od razu (co nie jest oczywiste - obiekt mógłby np. trzymać swoje dane w chmurze internetowej i nic nam do tego, a dając getName zakładamy, że odczyt danej to jest moment, a naprawdę będzie leciało zapytanie HTTP). Wiem, że często łatwiej założyć, że tak po prostu nigdy nie będzie i już - ale mimo wszystko, pełna enkapsulacja pozwalałaby na coś takiego.
  • właściwości da się nadpisać (czyli zakładamy mutowalność obiektu) - co jesli obiekt jest niemutowalny albo z jakiegoś innego powodu read only

Poza tym przy jakiejś deserializacji/klonowaniu obiektów to nie będzie zbyt wygodne - jechać i np. ustawiać z 20 właściwości, które są w obiekcie. Łatwiej byłoby przyjmować w konstruktorze dane obiektu (chociaż tu mogą dojść ograniczenia języków programowania - w JS można jedną linijką przekopiować dane z obiektu do obiektu, w innych językach nie wiem).

4
LukeJL napisał(a):

W obecnej postaci tak akurat jest. Z punktu widzenia enkapsulacji zasadnicze jest to, że mając metody dostępowe możesz zmienić tę implementację nie zmieniając API obiektu. K

Z jednej strony tak i na tym przykładzie pewnie to trafny kontrargument, ale jednak na nieco innym przykładzie już nie musi być tak kolorowo.

Ja nie mówię, że ta enkapsulacja jest dobrze zrobiona. Sprzeciwiłem się tylko, że nie ma jej tam w ogóle. Podstawy są.

Dając settery, zakładamy, że:

  • obiekt w ogóle ma pewne właściwości o określonym typie (a jeśli w przyszłości będziemy chcieli całkowicie zaorać struktury danych w danej klasie?)

To już dla zaawansowanych :) Można oczywiście stosować "domain specific types". Czyli zamiast String userName masz klasę UserName, która opakowuje sobie Stringa. Jeśli zajdzie potrzeba wprowadzenia jakichś zmian w tym, jak przechowywane jest UserName, wpływ tych zmian na kod - tzw. ripple effect - będzie ograniczony. Na dodatek kod staje się lepiej zabezpieczony przed błędami, bo numeru telefonu nie da się już przekazać jako nazwy użytkownika - zadba o to typowanie.

Poziom enkapsulacji bezsprzecznie wzrasta, tylko że enkapsulacja nie jest celem samym w sobie i trzeba pamiętać o YAGNI :) Żeby sobie nie skonstruować jakiejś kosmicznej architektury

  • właściwości da się uzyskać od razu (co nie jest oczywiste - obiekt mógłby np. trzymać swoje dane w chmurze internetowej i nic nam do tego, a dając getName zakładamy, że odczyt danej to jest moment, a naprawdę będzie leciało zapytanie HTTP). Wiem, że często łatwiej założyć, że tak po prostu nigdy nie będzie i już - ale mimo wszystko, pełna enkapsulacja pozwalałaby na coś takiego.

Ja szczerze mówiąc nie chciałbym pracować z kodem, gdzie getter w POJO odpalałby mi zapytanie HTTP :) To chyba nie enkapsulacja byłaby głównym problemem...

  • właściwości da się nadpisać (czyli zakładamy mutowalność obiektu) - co jesli obiekt jest niemutowalny albo z jakiegoś innego powodu read only

To jest bardzo trafna uwaga. Take note, @Władyslaw Parchomenko :) Zalecenie znane nie od dziś, także w świecie Javy: "classes should be immutable unless there's a very good reason to make them mutable" ("Effective Java"). Niemutowalność zwiększa enkapsulację praktycznie za darmo, przynosząc równocześnie szereg konkretnych korzyści - jak choćby lepsze bezpieczeństwo wątkowe.

Poza tym przy jakiejś deserializacji/klonowaniu obiektów to nie będzie zbyt wygodne - jechać i np. ustawiać z 20 właściwości, które są w obiekcie. Łatwiej byłoby przyjmować w konstruktorze dane obiektu

Co więcej, jeśli dojdzie nam kolejna właściwość - taka, że obiekt musi ją mieć ustawioną - to jeśli w którymś miejscu zapomnimy podać jej przez konstruktor, kod się w ogóle nie zbuduje.

A jeśli zapomnieliśmy gdzieś dołożyć kolejnego settera, wyjdzie to na jaw w formie różnych tajemnicznych zjawisk dopiero po czasie.

0

Kod ch*jowy , ale niech pierwszy rzuci kamieniem, kto na początku tak nie pisał :P

0
  1. Pakiet ma nazwę z d**y. Standard Java narzuca nazwy zaczynające się od odwróconej domeny.
  2. Użycie pól protected w klasie danych i dziedziczenie po niej to niedopuszczalny błąd. Ma być private i koniec.
  3. Jeżeli już używasz singletona, to rób to od początku do końca - w obecnej postaci możliwe jest utworzenie kilku klas Vladyslaw, jeśli będziesz używał więcej niż jednego wątku (brak synchronizacji)
  4. Kompilacja programu nie powiedzie się, bo z klasy Vladyslawnie masz dostępu do pól protected klasy Job.
  5. Dlaczego klasa Vladyslav musi być unikalna?
  6. Jaką dodatkową funkcjonalność wnosi Vladyslav? Przecież można zastąpić ją klasą Person i statyczną metodą tworzącą instancję Person wypełniającą od razu jej pola.
  7. W obecnej formie metoda showJobInfo mogłaby być sprowadzona do pojedynczej linijki wypisującej linijkę tekstu do konsoli.
  8. Klasy danych (Job, Person) wymagają po wywołaniu new dodatkowych zabiegów (podania danych). Lepiej byłoby użyć po prostu wieloargumentowego konstruktora.
  9. Jakakolwiek logika w metodzie main jest niedopuszczalna. Lepiej wywołać tam coś w stylu (new Application()).start()

To tak na szybko. Jak już ci napisano, podstawowym problemem tego kodu jest nic nierobienie i możliwość zastąpienia całości 2 linijkami - po co?

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