Czy ten kod jest "czysty" ?

Cześć, ćwiczę wraz z książką Roberta C. Martina "Czysty kod" jak pisać czysty kod, chciałbym aby chętni użytkownicy powiedzieli mi jak długo zajęło im zrozumienie co robi poszczególna metoda w tej klasie i czym jest poszczególne pole - ogólnie ile zajęło zrozumienie całego kodu. Z góry dzięki i pozdrawiam : )

package javaapplication5;

import java.util.Arrays;

public class Polynomial{

    private final int DEGREE;
    private final double[] COEFFICIENTS;
    

    public Polynomial(double ... coefficients){
        DEGREE = coefficients.length - 1;
        COEFFICIENTS = Arrays.copyOf(coefficients,coefficients.length);
    }
    
    
    public int getDegree(){
        return DEGREE;
    }
    
    
    public double[] getCoefficients(){
        return Arrays.copyOf(COEFFICIENTS, COEFFICIENTS.length);
    }
    
    
    public boolean checkIfNumberIsRoot(double x){
        return Double.compare(this.valueFor(x), 0) == 0;
    }

    public double valueFor(double x){
        double value = 0;
        
        for (int i =  0; i < COEFFICIENTS.length; i++)
            value += COEFFICIENTS[i]*Math.pow(x, COEFFICIENTS.length -1 - i);
        
        return value;
    }
}
 

Moim zdaniem kod wygląda zrozumiale. Jedynie bym zmienił upper case na lower case dla pół składowych. Upper case jest przeznaczony dla pół statycznych.

od razu wiadomo o co chodzi, ale pare rzeczy bym jednak zmienila (niektore sie wzajemnie wykluczaja):

• po co UPPERCASE dla czegokolwiek tutaj?
• klasa powinna byc final
• brak sprawdzania bledow w konstruktorze (co jak bedzie null podany?)
• jak juz robisz porownanie to po prostu valueFor(x) == 0 a nie cudowac z compare
• porownujac do double licz sie z tym ze nie ma on 100% dokladnosci
• duzo wydajniejszym algorytmem na obliczenie wartosci wielomianu jest schemat hornera
• ta czesc jest taka sobie Math.pow(x, COEFFICIENTS.length -1 - i), lepiej by to bylo ladnie ponazywac/przenies gdzie indziej

Odniosę się jeszcze do tych zaokrągleń. Używanie operatora przyrównania dla floatów/doubli jest bardzo złym pomysłem - tak jak już Katelx wspomniał, mają one swoje błędy. Gdziekolwiek używasz zmiennoprzecinkowych liczb, polecam BigDecimal (no chyba że nie będziesz robić przyrównań, tylko sprawdzać z grubsza zakres, to wtedy szybciej oczywiście na floatach/doublach).

Dzięki za odpowiedzi koledzy i koleżanki, dałem upper case bo to są stałe, ale macie rację dla satycznych pól przeznaczony jest upper case więc na pewno to zmienię, porównanie przez klasę Double musi być takie jakie jest, ponieważ porównywanie czy coś jest równe liczb zmiennoprzecinkowych nie jest do końca poprawne bo ani w double ani we float 0 nie jest wartością liczbową tylko wartością specjalną, a co do tego schematu hornera to też o tym myślałem i na pewno gdzieś to wydzielę zaraz i również zamienię klasę na final, a jeśli chodzi o te podawanie nulli albo jakby ktoś podał pustą tablicę albo współczynnik przy najwyższej potędze był równy zero to też to uwzględniłem (na razie w planach heh) i porobię dla tego osobne wyjątki, w książce o której mówiłem i na której się wzoruje autor bardzo poleca stosowanie wyjątków i serdecznie odradza sprawdzania kodów błędów itd. Jakbyście chcieli to wstawię drugi raz ten kod uwzględniąjacy wszystkie poprawki o których mówicie. BigDecimal nie jest zbyt wydajny moim zdaniem ale może jednak zmienię. Dzięki :)

No trochę pozmieniałem teraz, dodałem delikatny komentarz wskazujący, że jest obliczane to schematem Hornera (obsługę błędów jeszcze dodam później):

package javaapplication5;

import java.math.BigDecimal;
import static java.math.BigDecimal.ZERO;

public final class Polynomial{

    private final int degree;
    private final BigDecimal[] coefficients;
    

    public Polynomial(BigDecimal ... coefficients){
        degree = coefficients.length - 1;
        this.coefficients = coefficients.clone();
    }
    
    
    public int getDegree(){
        return degree;
    }
    
    
    public BigDecimal[] getCoefficients(){
        return coefficients.clone();
    }
    
    
    public boolean checkIfNumberIsRoot(BigDecimal x){
        return valueFor(x).equals(ZERO);
    }
    
    public BigDecimal valueFor(BigDecimal x){
        BigDecimal value = ZERO;
        
        for (BigDecimal coefficient : coefficients ) //Horner's method
            value = value.multiply(x).add(coefficient);
        
        return value;
    }
}

 

Wskazałeś na metodę obliczeń. Można by wynieść do osobnej metody, która miała by odpowiednią nazwę.

I co wtedy ta metoda valueFor miałaby tylko jedną linijkę ? jest sens robić takie coś ?

a odnosnie porównywania to zastosować należy compareTo ?

Kod (nazwy klas, metod) jest zawsze aktualniejszy od komentarzy, więc tak - jeśli to jest konieczne to warto pisać jednolinijkowce w metodzie.

Dzięki koledzy ! : )

Technicznie tak, będzie miała jedną linijkę. Jednocześnie będzie definiowała publiczne API klasy, które będzie niezależne od wewnętrznej implementacji. Zmienisz implementację, a klienci nawet tego nie poczują.

rozumiem, zapamiętam : )

Coś takiego jeszcze napisałem, pomyślałem,że te argumenty konstruktora nie są jasne bo właściwie nie wiadomo czym one są jak się zobaczy w kodzie tworzenie obiektu tej klasy, a taka metoda fabryczna z odpowiednią nazwą bardziej to rozjaśnia, spójrzcie:

package javaapplication5;

import java.math.BigDecimal;
import static java.math.BigDecimal.ZERO;

public final class Polynomial{
    private final int degree;
    private final BigDecimal[] coefficients;

    public static Polynomial FromCoefficients(BigDecimal ... coefficients){
        return new Polynomial(coefficients);
    }
    
    private Polynomial(BigDecimal[] coefficients){
        degree = coefficients.length - 1;
        this.coefficients = coefficients.clone();
    }
    
    
    public int getDegree(){
        return degree;
    }
    
    
    public BigDecimal[] getCoefficients(){
        return coefficients.clone();
    }
    
    
    public boolean checkIfNumberIsRoot(BigDecimal x){
        return computeValueFor(x).compareTo(ZERO) == 0;
    }
    
    public BigDecimal computeValueFor(BigDecimal x){
        return computeWithHornerAlgorithm(x);
    }
    
    private BigDecimal computeWithHornerAlgorithm(BigDecimal x){
        BigDecimal value = ZERO;
        
        for (BigDecimal coefficient : coefficients )
            value = value.multiply(x).add(coefficient);
        
        return value;
    }
}
 

tey, a czemu Pan używasz

...

, a nie []

 w parametrze metody? Jakoś mnie w dołku ściska jak coś takiego widzę :|

No większa swoboda, można podać całą tablicę a i można podawać pojedyncze obiekty BigDecimal.

No chyba, że chodzi Ci o parametr konstruktora prywatnego, to racja, to można usunąć i zamienić na wyłącznie typ tablicowy

ambrylun napisał(a):

No większa swoboda, można podać całą tablicę a i można podawać pojedyncze obiekty BigDecimal.

aaa, to tego mnie nie nauczyli, kudos!

No fajną to ma funkcjonalność.

Czy ja wiem? Z tymi ... są różne problemy jak ktoś poda nulla zamiast nie podawać argumentu na przykład. Ja bym w ogóle zrezygnował z tablic a jako argument dał jakieś Collection<Double>, chyba że zależy tu bardzo na wydajności i ktoś nie chce robić autoboxingu.

No jasne, że może podać nulla, każdy typ tablicowy to obiekt więc też może ale już mówiłem zresztą, że porobię wyjątki dla tych rzeczy, jak ktoś podałby pustą tablicę (albo kolekcję jak wolisz) to też to by był błąd albo podałby pierwszy współczynnik równy 0 to też, na wszystko porobi się wyjątki. Collection może też spoko, pomyślę.

Pusta tablica/kolekcja nie powinny zwracać błędu. Taki wielomian ma wartość 0 po prostu.

Nie, wielomian który ma wartość 0 to po prostu kolekcja z jednym elementem o wartości 0.