Miglioramento codice if/else con selezione voce menu

[fermat]

ciao!

vorrei migliorare un pò questo codice.
partiamo dal FileChooser:

public class DirChooser {

    private File lastDir;

    public DirChooser() {
        lastDir = new File(System.getProperty("user.home"));
    }

    public Optional<String> saveFile(Window ownerWindow, String title, FileChooser.ExtensionFilter ext) {
        FileChooser chooser = new FileChooser();
        chooser.setTitle(title);
        chooser.setInitialDirectory(lastDir);
        chooser.getExtensionFilters().add(ext);
        File file = chooser.showSaveDialog(ownerWindow);
        if (file != null) {
            return Optional.of(file.getPath());
        } else {
            return Optional.empty();
        }
    }

}

poi nel mio controller ho vari MenuItem per vari tipi di esportazione dati (json,xls,xlm,csv).
tutti richiamano lo stesso metodo, che fa una sorta di "dispatch".
a seconda dell'item selezionato, richiama il chooser e poi la classe preposta all'export:

@FXML
private MenuItem miCsv;

@FXML
private MenuItem miXls;

@FXML
private MenuItem miXml;

@FXML
private void esporta(ActionEvent ev) {
    Object obj = ev.getSource();

    List<Nota> list = null;
    try {
        list = db.getAll();
    } catch (SQLException ex) {
        GenericDialog.showDialog("Errore database: " + ex.getMessage(), Alert.AlertType.WARNING);
    }

    if (list != null) {
        DirChooser dc = new DirChooser();
        Optional<String> f;

        if (obj == miJson) {
            f = dc.saveFile(getStage().getOwner(), "Esporta JSON", new FileChooser.ExtensionFilter("JSON (*.json)", "*.json"));
            if (f.isPresent()) {
                try {
                    JsonDb jd = new JsonDb();
                    jd.create(f.get(), list);
                    GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
                } catch (IOException ex) {
                    GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
                }
            }
        } else if (obj == miXls) {
            f = dc.saveFile(getStage().getOwner(), "Esporta XLS", new FileChooser.ExtensionFilter("XLS (*.xls)", "*.xls"));
            if (f.isPresent()) {
                try {
                    ExcelDb ed = new ExcelDb();
                    ed.create(f.get(), tblCaption, list);
                    GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
                } catch (IOException | WriteException ex) {
                    GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
                }
            }
        } else if (obj == miCsv) {
            f = dc.saveFile(getStage().getOwner(), "Esporta CSV", new FileChooser.ExtensionFilter("CSV (*.csv)", "*.csv"));
            if (f.isPresent()) {
                try {
                    CsvDb cd = new CsvDb();
                    cd.create(f.get(), tblCaption, list);
                    GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
                } catch (IOException ex) {
                    GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
                }
            }
        } else if (obj == miXml) {
            f = dc.saveFile(getStage().getOwner(), "Esporta XML", new FileChooser.ExtensionFilter("XML (*.XML)", "*.XML"));
            if (f.isPresent()) {
                try {
                    XmlDb xd = new XmlDb();
                    xd.create(f.get(), list);
                    GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
                } catch (ParserConfigurationException | TransformerException ex) {
                    GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
                }
            }
        }
    }
}

in pratica, recupero la lista da database, e poi controllo quale item è stato cliccato.
in base a quello imposto i parametri del chooser, e richiamo la classe per l'export.
ma tutti questi if/else non mi piacciono.
secondo voi dove posso intervenire??

[andbin]

public class DirChooser {

    private File lastDir;

    public DirChooser() {
        lastDir = new File(System.getProperty("user.home"));
    }

    public Optional<String> saveFile(Window ownerWindow, String title, FileChooser.ExtensionFilter ext) {
        FileChooser chooser = new FileChooser();
        chooser.setTitle(title);
        chooser.setInitialDirectory(lastDir);
        chooser.getExtensionFilters().add(ext);
        File file = chooser.showSaveDialog(ownerWindow);
        if (file != null) {
            return Optional.of(file.getPath());
        } else {
            return Optional.empty();
        }
    }

}

Questa parte in generale va bene. Si potrebbero migliorare giusto solo alcuni piccoli aspetti:
- poter passare a saveFile più filter (rendi il metodo "varargs" e poi usi il addAll invece che add sul ObservableList dei filtri)
- restituire un Optional<File> invece che Optional<String>
- invece del if e poi of() oppure empty() puoi usare in un "colpo" solo ofNullable() di Optional

        if (obj == miJson) {
            f = dc.saveFile(getStage().getOwner(), "Esporta JSON", new FileChooser.ExtensionFilter("JSON (*.json)", "*.json"));
            if (f.isPresent()) {
                try {
                    JsonDb jd = new JsonDb();
                    jd.create(f.get(), list);
                    GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
                } catch (IOException ex) {
                    GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
                }
            }
        } else if (obj == miXls) {
            f = dc.saveFile(getStage().getOwner(), "Esporta XLS", new FileChooser.ExtensionFilter("XLS (*.xls)", "*.xls"));
            if (f.isPresent()) {
[....]

Sì, questa parte in effetti è meno "bella".
Una catena di if comunque ci vuole, perché devi "switchare" sui vari MenuItem. In teoria sarebbe anche possibile generalizzare la cosa, evitando gli if, ma servirebbe ben più codice e molto probabilmente non vale la pena.

Puoi però individuare cosa cambia e cosa non cambia tra i vari casi. Cambiano title e filtro, ma questi sono solo oggetti che puoi passare. Mentre invece cambia proprio il "comportamento" che c'è nel try. Questo lo potresti "catturare" con una tua functional interface.
E poi fai un metodo ancora a parte che date queste informazioni è generalizzato e fa tutto a partire dalla creazione del DirChooser.

[fermat]

ciao andbin!


allora, il DirChooser modificato:

public Optional<File> saveFile(Window ownerWindow, String title, FileChooser.ExtensionFilter... ext) {
    FileChooser chooser = new FileChooser();
    chooser.setTitle(title);
    chooser.setInitialDirectory(lastDir);
    chooser.getExtensionFilters().addAll(ext);
    File file = chooser.showSaveDialog(ownerWindow);
    return Optional.ofNullable(file);
}

invece, per function interface intendi una cosa del genere?

@FunctionalInterface
public interface DoExport {

    public void export();
}

perchè non le ho mai usate, così mi documento meglio!

[andbin]

allora, il DirChooser modificato:

Ok, bene.

invece, per function interface intendi una cosa del genere?

@FunctionalInterface
public interface DoExport {

    public void export();
}

Uhm ... no. Dipende anche da cosa vuoi "catturare" dell'ambiente esterno (cioè che non sia passato esplicitamente alla functional interface).

Ma direi di passargli almeno: a) il File, b) la lista, c) quel tblCaption (deduco sia una stringa) anche se non è usato in tutti i casi.

Quindi qualcosa del tipo:

@FunctionalInterface
public interface ListExporter<T> {
    void export(File file, List<T> list, String caption) throws IOException;
}

[fermat]

allora, sinceramente ho qualche problema / confusione.

partiamo dalla functional interface:

import jxl.write.WriteException;

import java.io.File;
import java.io.IOException;
import java.util.List;

@FunctionalInterface
public interface DoExport {

    public void export(File file, List<Nota> list, String[] header) throws IOException, WriteException;
}

ho aggiunto anche un'altra eccezione visto che uno degli export lo richiede.


poi nel controller:

@FXML
private void esporta(ActionEvent ev) {
    Object obj = ev.getSource();
    List<Nota> list = null;
    try {
        list = db.getAll();
    } catch (SQLException ex) {
        GenericDialog.showDialog("Errore database: " + ex.getMessage(), Alert.AlertType.WARNING);
    }

    if (list != null) {
        if (obj == miJson) {
            doExp(new JsonDb(), "Esporta JSON", new FileChooser.ExtensionFilter("JSON (*.json)", "*.json"), list);
        } else if (obj == miXls) {
        } else if (obj == miCsv) {
        } else if (obj == miXml) {
        }
    }
}

private <T> void doExp(T obj, String titolo, FileChooser.ExtensionFilter ext, List<Nota> list) {
    DirChooser dc = new DirChooser();
    Optional<File> file = dc.saveFile(getStage().getOwner(), titolo, ext);

    try {
        DoExport de = (File f, List<Nota> l, String[] header) -> f.getPath(), ll, str;
        de.export(file.get(), list, new String[]{});

    } catch (IOException | WriteException ex) {
        GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
    }
}

però, non saprei come richiamare il metodo di creazione dell'export che dovrei fare.
ammesso che ci abbia capito qualcosa ovviamente!

[fermat]

sto cercando di sbatterci la testa.
l'unica cosa che mi è venuta in mente, è usare la reflection.
ma ho dubbi che sia la strada giusta.

cmq:

private <T> void doExp(T t, String titolo, FileChooser.ExtensionFilter ext, List<Nota> list) {
    DirChooser dc = new DirChooser();
    Optional<File> file = dc.saveFile(getStage().getOwner(), titolo, ext);
    if (file.isPresent()) {
        try {
            DoExport de = (File f, List<Nota> l, String[] header) -> f.getPath(), ll, str;
            de.export(file.get(), list, new String[]{});

            Class<T> cl = (Class<T>) Class.forName(t.getClass().getName());
            Object obj = cl.getDeclaredConstructor().newInstance();
            Class[] params = new Class[2];
            params[0] = String.class;
            params[1] = List.class;
            Method method = cl.getDeclaredMethod("create", params);
            method.invoke(obj, file.get().getPath(), list);

        } catch (IOException | WriteException | ClassNotFoundException | NoSuchMethodException | IllegalAccessException | InstantiationException | InvocationTargetException ex) {
            GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
        }
    }
}

ma parte dal presupposto che tutti i vari tipi di export abbiamo il metodo create (che nel mio caso è così e mi va bene che sia cosi).
inoltre, non tutti gli export hanno gli stessi parametri.
ad esempio quello per l'export in CSV ha anche un String[] per l'intestazione.
quindi così scritto non è cmq flessibile.

[andbin]

l'unica cosa che mi è venuta in mente, è usare la reflection.

no noo

Innanzitutto, giusto perché non conosco il tuo codice, avrei io dei dubbi:
- cosa c'è in "comune" tra i vari JsonDb, ExcelDb, ecc...? Nulla? O si potrebbe pensare ad una interfaccia che li accomuna?
- i vari loro create() sono "generici" (in senso generalizzato)? O accettano solo List<Nota> ?

[fermat]

sostanzialmente:

public class JsonDb {

    public void create(String inputFile, List<Nota> list) throws IOException {

    }
}

public class XmlDb {
    public void create(String inputFile, List<Nota> list) throws ParserConfigurationException, TransformerException {

    }
}

public class CsvDb {

    public void create(String inputFile, String[] header, List<Nota> list) throws IOException {

    }
}

public class ExcelDb {

    public void create(String inputFile, String[] header, List<Nota> list) throws IOException, WriteException {

    }
}

quindi:
- tutti accettano il path dove salvare il file
- tutti accettano List<Nota> (io prevedo di usare solo questa lista in questo programma)
- alcuni accettano anche uno String[]
- alcuni hanno delle eccezioni "in più" rispetto ad altri

in comune, quindi, hanno praticamente solo che ricevono un path di salvataggio file, e una List<Nota>.
poi ogni export esporta in un determinato formato, usando diverse librerie.

[andbin]

- alcuni accettano anche uno String[]
- alcuni hanno delle eccezioni "in più" rispetto ad altri

Ok, chiarissimo ora. Il punto però è: le 4 classi possono anche non avere nulla in comune. Ma la functional interface che ho ipotizzato io serve proprio per "appiattire" le differenze tra i vari comportamenti. E il problema resta quello delle eccezioni. Sono ben diverse tra i vari create e questo influisce anche sulla functional interface, perché quelle eccezioni VANNO dichiarate (checked).

Una soluzione è mettere nella f.i. una eccezione più "ampia", es. Exception, sebbene non sia proprio il massimo:

@FunctionalInterface
public interface DoExport {
    void export(File file, List<Nota> list, String[] header) throws Exception;
}

Questo sarebbe ok, prendiamo per buono questo.

Quindi tra i vari export ci sarebbe di differente:
- il titolo (è un dato passabile)
- il filter (è un oggetto passabile)
- il "comportamento" specifico che usa una delle 4 classi.

Inizia con il fare nella tua classe dei metodi per ciascuna delle esportazioni, es.:

private void esportaNoteInJson(File file, List<Nota> list, String[] header) throws Exception {
    JsonDb jd = new JsonDb();
    jd.create(file.getAbsolutePath(), list);
}

Nota come la "forma" di questo metodo corrisponde a quella della f.i. (non conta il nome per le f.i.)


Poi fai il metodo che riceve quei parametri che configurano la esportazione:

private DirChooser dc = new DirChooser();    // così rimane l'ultima directory scelta

public void eseguiEsporta(List<Nota> list, String[] header, String titolo, FileChooser.ExtensionFilter filter, DoExport doExport) {
    Optional<File> f = dc.saveFile(getStage().getOwner(), titolo, filter);
    
    if (f.isPresent()) {
        try {
            doExport.export(f.get(), list, header);
            GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
        } catch (Exception ex) {
            GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
        }
    }
}

Poi nel esporta fai (abbozzato qui):

@FXML
private void esporta(ActionEvent ev) {
    // .....
    List<Nota> list = ......

        if (obj == miJson) {
            eseguiEsporta(list, tblCaption, "Esporta JSON",
                    new FileChooser.ExtensionFilter("JSON (*.json)", "*.json"),
                    this::esportaNoteInJson);
        }
        // ....
        
    // .....
}

Nota come al eseguiEsporta passo un method reference al esportaNoteInJson, che ha la forma del export di DoExport (la functional interface).


P.S. spero di non aver scritto errori, non ho visione completa sul tuo codice e ovviamente non posso "provare" nulla.

[fermat]

ok, adesso ho capito bene cosa indetevi per metodo generico!
ero partito in quarta con i generics senza ragionarci meglio sopra!

@FXML
private void esporta(ActionEvent ev) {
    Object obj = ev.getSource();
    List<Nota> list = null;
    try {
        list = db.getAll();
    } catch (SQLException ex) {
        GenericDialog.showDialog("Errore database: " + ex.getMessage(), Alert.AlertType.WARNING);
    }

    if (list != null) {
        if (obj == miJson) {
            eseguiEsporta(list, tblCaption, "Esporta JSON", new FileChooser.ExtensionFilter("JSON (*.json)", "*.json"), this::esportaNoteInJson);
        } else if (obj == miXls) {
        } else if (obj == miCsv) {
        } else if (obj == miXml) {
        }
    }
}

public void eseguiEsporta(List<Nota> list, String[] header, String titolo, FileChooser.ExtensionFilter filter, DoExport doExport) {
    Optional<File> f = dc.saveFile(getStage().getOwner(), titolo, filter);
    if (f.isPresent()) {
        try {
            doExport.export(f.get(), list, header);
            GenericDialog.showDialog("Database esportato!", Alert.AlertType.INFORMATION);
        } catch (Exception ex) {
            GenericDialog.showDialog(ex.getMessage(), Alert.AlertType.WARNING);
        }
    }
}

private void esportaNoteInJson(File file, List<Nota> list, String[] header) throws Exception {
    JsonDb ex = new JsonDb();
    ex.create(file.getAbsolutePath(), list);
}

private void esportaNoteInXml(File file, List<Nota> list, String[] header) throws Exception {
    XmlDb ex = new XmlDb();
    ex.create(file.getAbsolutePath(), list);
}

private void esportaNoteInCsv(File file, List<Nota> list, String[] header) throws Exception {
    CsvDb ex = new CsvDb();
    ex.create(file.getAbsolutePath(), header, list);
}

private void esportaNoteInXls(File file, List<Nota> list, String[] header) throws Exception {
    ExcelDb ex = new ExcelDb();
    ex.create(file.getAbsolutePath(), header, list);
}

per quanto riguarda le f.i., devo ancora capire bene, perchè il tuo esempio è molto diverso da quelli che ho visto in giro.
cmq mi pare di capire che creando i metodi con la stessa "forma" del metodo della f.i., posso usare questo "modo":

this::metodo

infatti, anche dove non necessario, hai aggiunto il parametro String[] header (ad esempio nella esportazione in json).
tanto più che, se lo levo, mi da errore.
più o meno corretto??