Gestione ordinamento di una tabella di dati

[Ansharja]

Allora per adesso ho risistemato il codice in questo modo:

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
public class TableSort
{
    boolean descending;
    int sort;
    Comparator<Game> comparator;
    public TableSort(int s,boolean d)
    {
        sort=s;
        descending=d;
    }
    public void sort(ArrayList<Game> games)
    {
        if(sort==0)comparator=new PathComparator();
        else if(sort==1)comparator=new DateComparator();
        else if(sort==2)comparator=new WhiteComparator();
        else if(sort==3)comparator=new BlackComparator();
        else if(sort==4)comparator=new WhiteRankComparator();
        else if(sort==5)comparator=new BlackRankComparator();
        else if(sort==6)comparator=new TypeComparator();
        else if(sort==7)comparator=new CommentComparator();
        else return;
        if(descending)comparator=Collections.reverseOrder(comparator);
        Collections.sort(games,comparator);
    }
    class PathComparator implements Comparator<Game>
    {
        public int compare(Game g,Game h){
            return g.path.compareTo(h.path);
        }
    }
    class DateComparator implements Comparator<Game>
    {
        public int compare(Game g,Game h){
            return g.date.compareTo(h.date);
        }
    }
    class WhiteComparator implements Comparator<Game>
    {
        public int compare(Game g,Game h){
            return g.white.compareTo(h.white);
        }
    }
    // ... stessa storia per gli altri comparatori
}

Va bene come design ?

[andbin]

Allora per adesso ho risistemato il codice in questo modo

Ok, già meglio di prima sicuramente, non ci sono campi static.
La classe però è molto migliorabile e per diversi aspetti.

- Ti è già stato suggerito sopra l'uso di un switch, questo aiuta sicuramente a rendere più pulito (ma anche più efficiente) il codice.
- Se usassi delle costanti letterali sarebbe ancora meglio e più leggibile, specialmente nel/nei punto/i dove istanzi i TableSort. Invece che 6 es. TYPE_ORDER. Se le facessi come enum (che con lo switch sono ottime) sarebbe ancora meglio.
- Come suggerimento, abituati a tenere le variabili di istanza generalmente come "private", salvo casi (e certi design) davvero particolari. Quelle di TableSort non sono private. E non lo sono nemmeno quelle in Game, visto che fai g.white, g.date ecc.... Puoi accedere ai campi anche dalle classi dei comparator, perché quelle variabili sono sicuramente non private. Con le variabili di istanza private, devi definire degli appositi metodi getter/setter.
- Le classi dei comparatori tecnicamente le hai fatte come "regular" inner-class. Una inner-class ha una relazione molto particolare con la classe che la contiene. Nel tuo caso questo non serve. Puoi farle come classi top-level a sé stanti. Oppure tenerle dentro TableSort ma farle come "nested" (static) class che sono semplicemente marcate static.
- Non mi piace tantissimo il fatto che la scelta del comparator la fai nel tuo metodo sort ma non ho capito se volevi fare la classe TableSort per avere sue istanze mutabili o immutabili. Questo farebbe la differenza. Se fosse immutabile, la scelta del comparator la farei nel costruttore.

[Ansharja]

Grazie ancora per la pazienza!

- Ti è già stato suggerito sopra l'uso di un switch, questo aiuta sicuramente a rendere più pulito (ma anche più efficiente) il codice.
- Se usassi delle costanti letterali sarebbe ancora meglio e più leggibile, specialmente nel/nei punto/i dove istanzi i TableSort. Invece che 6 es. TYPE_ORDER. Se le facessi come enum (che con lo switch sono ottime) sarebbe ancora meglio.

Alle costanti letterali avevo in realtà già pensato, visto che con Swing ne faccio molto uso, modificherò a breve la cosa.
Confesso invece la mia ignoranza su enum e switch: a me sembrava terribile il dover ripetere la clausola break per uscire dai vari case, ma se dite che è più performante implemento quella soluzione.
Le enum non le ho proprio mai usate invece, altra cosa da guardarmi

- Come suggerimento, abituati a tenere le variabili di istanza generalmente come "private", salvo casi (e certi design) davvero particolari. Quelle di TableSort non sono private. E non lo sono nemmeno quelle in Game, visto che fai g.white, g.date ecc.... Puoi accedere ai campi anche dalle classi dei comparator, perché quelle variabili sono sicuramente non private. Con le variabili di istanza private, devi definire degli appositi metodi getter/setter.

Per quanto riguarda la definizione dei campi come private e l'uso dei metodi getter/setter, ho già letto (anche su altri forum) i vantaggi di questo approccio, dovrei essere meno testardo ed adeguarmi.

- Le classi dei comparatori tecnicamente le hai fatte come "regular" inner-class. Una inner-class ha una relazione molto particolare con la classe che la contiene. Nel tuo caso questo non serve. Puoi farle come classi top-level a sé stanti. Oppure tenerle dentro TableSort ma farle come "nested" (static) class che sono semplicemente marcate static.

Ricevuto, volevo solo evitare la sovrabbondanza di file, se i due approcci sono equiparabili le segnerò come static

- Non mi piace tantissimo il fatto che la scelta del comparator la fai nel tuo metodo sort ma non ho capito se volevi fare la classe TableSort per avere sue istanze mutabili o immutabili. Questo farebbe la differenza. Se fosse immutabile, la scelta del comparator la farei nel costruttore.

Non mi è chiarissimo questo punto, TableSort la utilizzerei solo per l'ordinamento...
Dici di inserire solo la scelta del Comparator nel costruttore o inglobarci anche il resto del metodo sort() ?

Penso la prima comunque, mi ricordo di aver letto un tuo consiglio sul fatto di non demandare troppa logica a un costruttore.
Se ho capito bene, sistemo il tutto.

Grazie ancora

[andbin]

Rispondo ora ... me ne stavo dimenticando.

a me sembrava terribile il dover ripetere la clausola break per uscire dai vari case, ma se dite che è più performante implemento quella soluzione.

La differenza di performance tra catena di if e switch in quel caso è assolutamente irrilevante. I casi di valori che hai sono pochi e soprattutto quel metodo non lo esegui certo 10000 volte al secondo!

Riguardo i break, ok, però nessuno ti vieta di rifattorizzare un pochino il codice e mettere un metodo privato che fornisce solo il comparator a fronte del sort. A quel punto puoi usare i return così:

private Comparator<Game> createComparator() {
    switch (sort) {
    case 0: return new PathComparator();
    case 1: return new DateComparator();
        //.....
    default: throw new IllegalStateException("Invalid sort state");
    }
}

Con il risultato di tenere separati ordinamento e scelta dell'ordinamento, rendendo più breve appunto il sort.

Non mi è chiarissimo questo punto, TableSort la utilizzerei solo per l'ordinamento...
Dici di inserire solo la scelta del Comparator nel costruttore o inglobarci anche il resto del metodo sort() ?

Intendevo dire che puoi scegliere se:
- avere TableSort immutabile, ogni volta che serve un nuovo criterio, crei una nuova istanza.
- avere TableSort mutabile, fornendo dei metodi setter (eventualmente getter ma non è strettamente necessario) per cambiare lo stato dell'oggetto.

Ma dipende anche da come usi gli oggetti TableSort. Se solo all'interno di una tua classe crei e usi oggetti TableSort, è abbastanza indifferente. Hai tu il controllo completo dell'oggetto, che non "esce fuori". Se invece dovessi passare un oggetto TableSort ad un'altra classe, in linea di massima e senza sapere o vedere altro, sarebbe preferibile l'approccio immutabile, perché così l'oggetto è tranquillamente thread-safe e condivisibile.
Insomma, questo è uno di quei aspetti di design che ovviamente vanno valutati caso per caso.