Gestire apertura file con eventuale exception

[roquentin]

Ciao a tutti, devo gestire l'errore derivante dall'apertura di un file nel caso questo sia vuoto oppure non presenti i dati esatti :

private  String parseString(final BufferedReader reader, final String name1,  final String name2, final boolean skipLast) throws  DatiMeseInvalidiException {
        try {
            final String input = reader.readLine();

             if (input != null && !input.trim().isEmpty() &&  (input.startsWith(name1) || input.startsWith(name2))) {
                int index = input.indexOf(":");
                if (index < 0) {
                    throw new DatiMeseInvalidiException("Identificatore non presente");
                }
                if (skipLast) {
                    int indexV = input.indexOf(",");
                    return input.substring(index + 1, indexV);
                } else {
                    return input.substring(index + 1);
                }
            }
            throw new DatiMeseInvalidiException("Identificatore non presente");
        } catch (IOException ex) {
            Logger.getLogger(ApriFileMese.class.getName()).log(Level.SEVERE, null, ex);
        }
        return null;
    }

questo è il metodo contenuto nella classe per aprire il file che mi va ad analizzare il contenuto di un file .txt analizzandone le righe.

Se non trova i dati nell'ordine prestabilito o le righe sono vuote lancia una eccezione:

public class DatiMeseInvalidiException extends Exception {

    public DatiMeseInvalidiException(String message) {
        super(message);
        JOptionPane.showMessageDialog(null,
                "It's impossible to get the information.",
                "Warning",
                JOptionPane.WARNING_MESSAGE);
        
    }
}

Il problema è che una volta visualizzato il messaggio tramite JOptionPane, mi da errore, ovviamente

....
SEVERE: null
pv.DatiMeseInvalidiException: Identificatore non presente
...

ed io vorrei che una volta clicclato su "Ok" nel JOptionPane del Warning Message, si bloccasse l'esecuzione senza segnalazione di errore.

Come posso risolvere ?

Grazie

[andbin]

Innanzitutto c'è una questione fondamentale: nella classe di una eccezione non va mai fatta alcuna interazione con l'utente ... tanto meno con una message box. Quindi rivedi e ripensa bene il tutto.

[roquentin]

Innanzitutto c'è una questione fondamentale: nella classe di una eccezione non va mai fatta alcuna interazione con l'utente ... tanto meno con una message box. Quindi rivedi e ripensa bene il tutto.

Perchè nella classe di una eccezione non va mai fatta alcuna interazione con l'utente ?

[roquentin]

Ho pensato di eliminare

throw new DatiMeseInvalidiException("Identificatore non presente");

inserendo un controllo sul valore di ritorno della classe, per esempio:

private String parseString(final BufferedReader reader, final String  name1, final String name2, final boolean skipLast) throws  DatiMeseInvalidiException {
        try {
            final String input = reader.readLine();

             if (input != null && !input.trim().isEmpty() &&  (input.startsWith(name1) || input.startsWith(name2))) {
                int index = input.indexOf(":");
                if (index < 0) {
                    return null;
                }
                if (skipLast) {
                    int indexV = input.indexOf(",");
                    return input.substring(index + 1, indexV);
                } else {
                    return input.substring(index + 1);
                }
            }
           return null;
        } catch (IOException ex) {
            Logger.getLogger(ApriFileMese.class.getName()).log(Level.SEVERE, null, ex);
        }
        return null;
    }

e quindi se String == null, mi mostra il warning message..che ne dici?

[andbin]

Perchè nella classe di una eccezione non va mai fatta alcuna interazione con l'utente ?

Perché una eccezione è solamente un semplice oggetto che viene istanziato, lanciato e poi catturato altrove, potenzialmente anche molto più a monte di dove è stata scatenata la eccezione. Non è la classe della eccezione che deve interagire con l'utente!

Una eccezione può eventualmente contenere informazioni extra, pertinenti al motivo della eccezione. Prendi come esempio una ipotetica ConfigurationKeyNotFoundException, una informazione extra potrebbe essere appunto la 'key' che non è stata trovata. Quindi potrebbe essere una cosa tipo:

public class ConfigurationKeyNotFoundException extends Exception {
    private String key;

    public ConfigurationKeyNotFoundException(String key) {
        super(String.format("Configuration key '%s' not found", key));
        this.key = key;
    }

    public String getKey() {
        return key;
    }
}

Questo va bene, è assolutamente appropriato.

Ma le eccezioni, in generale:
- Non devono essere usate solo per "passare" informazioni da un punto ad un altro (più a monte) giusto solo perché farebbe comodo (sarebbe un abuso e non è quello l'obiettivo delle eccezioni).
- Non devono fare I/O in generale, a meno che sia pertinente alla eccezione, ad esempio per "localizzare" il messaggio.
- Non devono interagire con l'utente in alcun modo, meno che mai con delle message-box grafiche.


Se la eccezione è "importante" e oltretutto checked, dovrà essere catturata e gestita prima o poi da qualche parte a monte, e se dove gestisci la eccezione c'è una nozione chiara del contesto e di come deve avvenire l'interazione con l'utente, allora lì sì, puoi aprire una message-box di errore.

Già ... il problema è appunto tenere ben separati i concetti. Quel tuo metodo 'parseString' dove è messo? Visto che hai un file da gestire e con un formato ben preciso, mi aspetterei che tu abbia fatto una classe a sé stante che si occupa solo della lettura/parsing del file. Hai fatto così? O hai fatto un bel miscuglio di concetti in una unica classe?

Ho pensato di eliminare

throw new DatiMeseInvalidiException("Identificatore non presente");

inserendo un controllo sul valore di ritorno della classe

che ne dici?

Non, è peggio! Innanzitutto un 'null' molte volte è ambiguo e perlomeno dovresti documentare (senza che qualcuno debba per forza guardarsi il tuo codice e capire la logica) cosa significa un null restituito da parseString. Poi comunque la possibilità di un null, forza il chiamante a fare almeno un test, quindi ... più codice.
Inoltre null è solo uno, non hai altri valori speciali, quindi se ci sono diverse problematiche da segnalare, usare sempre e solo "null" è ovviamente molto restrittivo.

E infine, il fatto che a parseString passi un BufferedReader, dà abbastanza (a me perlomeno) l'idea che stai ragionando poco "ad oggetti".


E tutto questo che stai dicendo è quasi certamente un segno delle tue parole dei giorni scorsi: "Purtroppo non riesco ancora ad entrare nella logica del linguaggio ad oggetti...ma ci riuscirò"

[roquentin]

Questa è la classe che utilizza il metodo e che comprende il metodo per parsare le stringhe:

public class ApriFileIrraggiamento {

    public Irraggiamento apriFiles(File directory) throws FileNotFoundException, IOException, DatiMeseInvalidiException {
        
        FileReader f = new FileReader(directory + File.separator + "Radiation.txt");
        BufferedReader b = new BufferedReader(f);
        Irraggiamento irraggiamento = new Irraggiamento();
        CalcolaMediaIrraggiamentoAnnuale mediaAnnuale = new CalcolaMediaIrraggiamentoAnnuale();
        List<RegistrazioniIrraggiamenti> registrazioni = new ArrayList<>();
        
        try {
            String latitude = parseString(b, "Lat", "Lati", true);
            String longitude = parseString(b, "Long", "Longi", false);
            b.readLine();// Potenza nominale del sistema FV
            String inclinazione = parseString(b, "Incli", "Inclin", false);
            String azimuth = parseString(b, "Orie", "Orient", false);
            b.readLine(); // empty
            b.readLine(); // Angolo fisso    
            String[] intestazione = b.readLine().split("\\s+");         // intestazione
            if (intestazione.length >= 1) {
                    int i;
                    for (i = 0; i < intestazione.length; i++) {
                        System.out.print(intestazione[i] + "\t");

                    }
                    System.out.println();
                }
            Coordinate coordinate = new Coordinate(latitude.trim(), longitude.trim(), inclinazione.trim(), azimuth.trim());
            irraggiamento.setCoordinate(coordinate);
            String linea = b.readLine();
            int i;
            for (i = 0; i < 12; i++) {
                if (linea != null && !linea.equals("")) {
                     RegistrazioniIrraggiamenti registro = new  RegistrazioniIrraggiamenti(linea.trim(), intestazione,  intestazione.length);
                    registrazioni.add(registro);
                    linea = b.readLine();
                }
            }
            irraggiamento.setRegistrazioniAnno(registrazioni);
            double mediaHm = mediaAnnuale.calcolaMediaIrraggiamentoAnno(irraggiamento);
            irraggiamento.setIrraggiamentoAnnualeHm(mediaHm);
        } finally {
            try {
                b.close();
            } catch (IOException e) {
            }
        }
        return irraggiamento;
    }
    
     private String parseString(final BufferedReader reader, final String  name1, final String name2, final boolean skipLast) throws  DatiMeseInvalidiException {
        try {
            final String input = reader.readLine();

             if (input != null && !input.trim().isEmpty() &&  (input.startsWith(name1) || input.startsWith(name2))) {
                int index = input.indexOf(":");
                if (index < 0) {
                    throw new DatiMeseInvalidiException("Identificatore non presente");
                }
                if (skipLast) {
                    int indexV = input.indexOf(",");
                    return input.substring(index + 1, indexV);
                } else {
                    return input.substring(index + 1);
                }
            }
            throw new DatiMeseInvalidiException("Identificatore non presente");
        } catch (IOException ex) {
            Logger.getLogger(ApriFileMese.class.getName()).log(Level.SEVERE, null, ex);
        }
        return null;
    }
}

Come è visibile nella classe vado a parsare le righe del file txt per estrarne le informazioni utili. A questo punto, devo valutare il caso in cui il file è vuoto o parzialmente vuoto o comunque non riesco ad estrare le informazioni che mi servono.

Che consiglio mi daresti? Come dovrei procedere?


Grazie

[andbin]

Che consiglio mi daresti? Come dovrei procedere?

Il fatto di aver creato una classe a parte (ApriFileIrraggiamento) solo per la gestione del file, ok, è buono.

Se Irraggiamento rappresenta tutto il file (e mi pare di capire che è così), allora il fatto che parseString riceva il BufferedReader, tutto sommato potrebbe anche andare bene, perché tutta la gestione è in apriFiles e quindi per forza glielo deve passare.
Ma se ci pensi, e ragioni, parseString legge solo una riga e fa il parsing di quella riga. Quindi NON è affatto necessario che parseString riceva il BufferedReader. Basta che riceva la stringa.
Il parseString viene chiamato 4 volte da apriFiles e tu per evitare 4 readLine(), per questo hai passato il BufferedReader. Ma potevi fare es.

String latitude = parseString(b.readLine(), "Lat", "Lati", true);
......

Che costava molto poco ....

Quello che va meno bene è che in parseString catturi IOException, fai un log (e questo va bene) ma poi finisce lì. No, quel IOException dovrebbe poter uscire da parseString e anche da apriFiles.


Ah, qui ad esempio:

String[] intestazione = b.readLine().split("\\s+"); // intestazione

non è molto buono: se readLine() restituisce null, ti becchi NullPointerException. Ma se ti dà null, vuol dire che sei a fine file e se quella riga te la aspettavi, vuol dire che il file è "malformato" e lo devi segnalare opportunamente.
Dovresti valutare questo aspetto in generale, anche per le altre readLine()

[roquentin]

Il fatto di aver creato una classe a parte (ApriFileIrraggiamento) solo per la gestione del file, ok, è buono.

Se Irraggiamento rappresenta tutto il file (e mi pare di capire che è così), allora il fatto che parseString riceva il BufferedReader, tutto sommato potrebbe anche andare bene, perché tutta la gestione è in apriFiles e quindi per forza glielo deve passare.
Ma se ci pensi, e ragioni, parseString legge solo una riga e fa il parsing di quella riga. Quindi NON è affatto necessario che parseString riceva il BufferedReader. Basta che riceva la stringa.
Il parseString viene chiamato 4 volte da apriFiles e tu per evitare 4 readLine(), per questo hai passato il BufferedReader. Ma potevi fare es.

String latitude = parseString(b.readLine(), "Lat", "Lati", true);
......

Che costava molto poco ....

In effetti hai pienamente ragione ed ho fatto così:

private String parseString(final String input, final String name1, final String name2, final boolean skipLast) {
        
        if (input != null && !input.trim().isEmpty() && (input.startsWith(name1) || input.startsWith(name2))) {
            int index = input.indexOf(":");
            if (skipLast) {
                int indexV = input.indexOf(",");
                return input.substring(index + 1, indexV);
            } else {
                return input.substring(index + 1);
            }
        }
        return null;
    }

Quello che va meno bene è che in parseString catturi IOException, fai un log (e questo va bene) ma poi finisce lì. No, quel IOException dovrebbe poter uscire da parseString e anche da apriFiles.

Passando non il buffereader ma la singola stringa a ParseString, mi segnalava errore sulla catch cause

exception IOException is never thrown in body of corresponding try statement

Ah, qui ad esempio:

String[] intestazione = b.readLine().split("\\s+"); // intestazione

non è molto buono: se readLine() restituisce null, ti becchi NullPointerException. Ma se ti dà null, vuol dire che sei a fine file e se quella riga te la aspettavi, vuol dire che il file è "malformato" e lo devi segnalare opportunamente.
Dovresti valutare questo aspetto in generale, anche per le altre readLine()

Infatti ho notato che se mi restituisce null, ho NullPointerException se non trova quella riga ma non capisco cosa vuoi dire con

e se quella riga te la aspettavi, vuol dire che il file è "malformato" e lo devi segnalare opportunamente.

[andbin]

Passando non il buffereader ma la singola stringa a ParseString, mi segnalava errore sulla catch cause

Se parseString non riceve più BufferedReader, allora chiaramente non serve più nemmeno il try-catch, visto che nulla lì dentro potrebbe lanciare IOException

Infatti ho notato che se mi restituisce null, ho NullPointerException se non trova quella riga ma non capisco cosa vuoi dire con

Se arrivi a quel punto e quella riga ti aspetti che ci sia sempre (non è opzionale) .... e invece non c'è (perché sei a end-of-file), vuol dire che il file è malformato. Dovresti segnalarlo con una eccezione.
E in generale, dovresti fare questi ragionamenti per ogni riga che è da ritenersi fondamentale per il formato del file.

[roquentin]

Se arrivi a quel punto e quella riga ti aspetti che ci sia sempre (non è opzionale) .... e invece non c'è (perché sei a end-of-file), vuol dire che il file è malformato. Dovresti segnalarlo con una eccezione.
E in generale, dovresti fare questi ragionamenti per ogni riga che è da ritenersi fondamentale per il formato del file.

Infatti è proprio questo il punto..come faccio a segnalare con l'eccezione ed indicare all'utente che il file non è nel formato previsto (è completamente vuoto o manca quella detterminata stringa in quella determinata posizione) ????