Aiuto su desgin di una classe

[fermat]

ciao!

ho una classe che usa Apache Tika per fare il parsing dei file in base al mime type.

public class ParseFile {

    private File file;
    private BodyContentHandler handler;
    private Metadata metadata;
    private ParseContext context;
    private FileInputStream inputstream;
    private StringBuilder sb;
    private String[] metadataNames;
    private String content;

    public ParseFile(File file) throws FileNotFoundException {
        this.file = file;
        handler = new BodyContentHandler();
        metadata = new Metadata();
        context = new ParseContext();
        inputstream = new FileInputStream(file);
        sb = new StringBuilder();
    }

    public void genericParser() throws IOException, SAXException, TikaException {
        Parser parser = new AutoDetectParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void jpegParser() throws IOException, SAXException, TikaException {
        JpegParser parser = new JpegParser();
        parser.parse(inputstream, handler, metadata, context);
        setMetadataNames(metadata.names());
    }

    public void ooXmlParser() throws IOException, SAXException, TikaException {
        OOXMLParser parser = new OOXMLParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }
..................................................................
}

come vedete ho creato un metodo per ogni tipo di parsing che mi interessa.
da una JTable prendo il file e lo passo al costruttore.
però, in sostanza, sono tutti metodi praticamente uguali, e mi sembra un pò una ripetizione.
cambia solo il parser da utilizzare.
come potrei migliorare questa classe secondo voi?

[andbin]

come potrei migliorare questa classe secondo voi?

Per poter valutare un design migliore, bisognerebbe comprendere il contesto d'uso. Chi usa ParseFile? Chi invoca i vari genericParser ecc.. e in base a cosa? Dove è l' "output" di tutto questo? (non conosco Tika e non riesco a capire cosa fa .. vedo i metodi void e senza argomenti ... quindi cosa "produce"?)

[fermat]

ciao andbin.

si hai ragione mi spiego meglio.
ho una lista di file messa dentro una JTable.
al doppio click lancio il parsing passando il path completo del file.
a quel punto apro un'altra finestra, dove:
- nella parte di sinistra ho i metadata
- nella parte destra il JPanel cambia a seconda del tipo di file (se è una immagine la visualizza, se è un documento di testo mostra il contenuto, ecc).
quindi questa la classe ParseFile completa:

public class ParseFile {

    private File file;
    private BodyContentHandler handler;
    private Metadata metadata;
    private ParseContext context;
    private FileInputStream inputstream;
    private StringBuilder sb;
    private String[] metadataNames;
    private String content;

    public ParseFile(File file) throws FileNotFoundException {
        this.file = file;
        handler = new BodyContentHandler();
        metadata = new Metadata();
        context = new ParseContext();
        inputstream = new FileInputStream(file);
        sb = new StringBuilder();
    }

    public void genericParser() throws IOException, SAXException, TikaException {
        Parser parser = new AutoDetectParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void jpegParser() throws IOException, SAXException, TikaException {
        JpegParser parser = new JpegParser();
        parser.parse(inputstream, handler, metadata, context);
        setMetadataNames(metadata.names());
    }

    public void ooXmlParser() throws IOException, SAXException, TikaException {
        OOXMLParser parser = new OOXMLParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void msOfficeParser() throws IOException, SAXException, TikaException {
        OfficeParser parser = new OfficeParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void pdfParser() throws IOException, SAXException, TikaException {
        PDFParser parser = new PDFParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void rtfParser() throws IOException, SAXException, TikaException {
        RTFParser parser = new RTFParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void xmlParser() throws IOException, SAXException, TikaException {
        XMLParser parser = new XMLParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void htmlParser() throws IOException, SAXException, TikaException {
        HtmlParser parser = new HtmlParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void openDocParser() throws IOException, SAXException, TikaException {
        OpenDocumentParser parser = new OpenDocumentParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void oggParser() throws IOException, TikaException, SAXException {
        VorbisParser parser = new VorbisParser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    public void mp3Parser() throws IOException, TikaException, SAXException {
        Mp3Parser parser = new Mp3Parser();
        parser.parse(inputstream, handler, metadata, context);
        setContent(handler.toString());
        setMetadataNames(metadata.names());
    }

    private void setContent(String content) {
        this.content = content;
    }

    public String getContent() {
        return content;
    }

    private void setMetadataNames(String[] metadataNames) {
        this.metadataNames = metadataNames;
        for (String name : metadataNames) {
            sb.append(name).append(": ").append(metadata.get(name)).append("\n");
        }
    }

    public String getMetadataNames() {
        return sb.toString();
    }
}

questo l'evento al doppio click sulla JTable:

            File f = new File(strFile);
            String type = strType;
            ParseFile pf = new ParseFile(f);
            WindowDetail wd = null;
            switch (type) {
                case "image/jpeg":
                case "image/jpg":
                    pf.jpegParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelImage(f));
                    break;
                case "application/vnd.ms-excel":
                case "application/msword":
                case "plication/vnd.ms-powerpoint":
                    pf.msOfficeParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/vnd.openxmlformats-officedocument.wordprocessingml.document":
                case "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet":
                case "application/vnd.openxmlformats-officedocument.presentationml.presentation":
                    pf.ooXmlParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/rtf":
                    pf.rtfParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/xml":
                    pf.xmlParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "text/html":
                    pf.htmlParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/vnd.oasis.opendocument.text":
                case "application/vnd.oasis.opendocument.spreadsheet":
                case "application/vnd.oasis.opendocument.presentation":
                    pf.openDocParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/pdf":
                    pf.pdfParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "application/ogg":
                    pf.oggParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                case "audio/mpeg":
                    pf.mp3Parser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
                default:
                    pf.genericParser();
                    wd = new WindowDetail(f, new PanelDetail(pf.getMetadataNames()), new PanelDetail(pf.getContent()));
                    break;
            }
            wd.setVisible(true);

WindowDetail è un JFrame a cui passo i JPanel da visualizzare, ed il realtivo contenuto (metadati, immagine, ecc).

solo che così mi sembra un pò tutto un accrocco.

[andbin]

Premessa: innanzitutto per me non è molto facile darti indicazioni esatte e risolutive, non per mancanze mio a livello di design ma perché chiaramente non conosco i concetti usati da Tika, non conosco quelle sue classi specifiche ecc...

La prima cosa che noto è che ParseFile adesso è una classe abbastanza grande e con uno "stato" abbastanza complesso e oltretutto "mutabile" (già solo per quel StringBuilder). Questo significa sicuramente che una istanza di ParseFile la puoi usare solo per un singolo file e poi la puoi buttare via.

Uno dei principi della OOP è di cercare di separare cosa cambia da cosa non cambia. Cosa c'è di comune in tutto quell'algoritmo di parsing? E cosa è differente in base al tipo di file?
L'ideale sarebbe avere degli oggetti con stato "immutabile" o addirittura stateless (ma in questo tuo caso non è detto sia possibile), così un oggetto della classe di parsing lo puoi riusare e magari tenere "registrato" in un'altra classe associandolo a più content-type, estraendolo "per tipo" ogni volta che ti serve.

Quindi inizia a pensare ad una classe base in cui ci sono le parti comuni e poi a sottoclassi specifiche, per ciascuna tipologia di file. Se questo "algoritmo" di parsing è fatto di più fasi e alcune di queste sono specifiche per il tipo di file, sfrutta il pattern "Template".
Questo vuol anche dire che dovrai sfruttare il principio di override.

Inoltre fai in modo che il "risultato" non sia incapsulato nell'oggetto di parsing (come è adesso) ma sia restituito come nuovo oggetto.
Alla fine l'utilizzo di tutto quanto potrebbe essere una cosa del tipo:

Da qualche parte, una-tantum nella applicazione:

FileParserRegistry fileParserRegistry = new FileParserRegistry();
// registrazione delle associazioni tipo -> oggetto di parsing
// es.:
// fileParserRegistry.add(new JPEGFileParser(), "image/jpeg", "image/jpg");
// ...
// fileParserRegistry.add(new RTFFileParser(), "application/rtf");
// ...

Poi dove dovrai parsare il file:

FileParser fileParser = fileParserRegistry.getForType(fileType);
FileParseResult result = fileParser.parse(file);

E tutto il tuo mega-switch svanisce e al suo posto ci sono poche righe. Se dovessi poi aggiungere una tipologia, dovrai poi solo creare una nuova classe di parser (ovviamente) e registrarla.

Prova a pensare in questo senso. Poi potremmo valutare meglio se hai dubbi.

[fermat]

ciao andbin!

step by step.

allora, mi sono documentato sul pattern Template.
questo prevede una classe astratta, che nel mio caso dovrebbe fare una cosa del genere se ho ben capito:

public abstract class ParserTemplate {

    // INIZIALIZZO LE PARTI "COMUNI"
    abstract void initialize();
    
    // PRENDO I METADATA
    abstract String getMetadata();

    // PRENDO IL CONTENUTO
    abstract String getContent();

    public final void init() {
        initialize();
        getMetadata();
        getContent();
    }
}

poi ci sono delle classi concrete che estendono quella classe astratta.
che dovrebbero essere i miei parser, tipo:

public class RTFParser extends ParserTemplate {

    @Override
    void initialize() {
        
    }

    @Override
    String getMetadata() {
        
    }

    @Override
    String getContent() {
        
    }

}

quante catronerie ho detto??

per quanto riguarda il FileParserRegistry, non ho ben capito.
utilizza un altro pattern sul quale posso documentarmi?

[andbin]

allora, mi sono documentato sul pattern Template.
questo prevede una classe astratta, che nel mio caso dovrebbe fare una cosa del genere se ho ben capito:
quante catronerie ho detto??

Partiamo dall'uso "ipotetico" che ho detto prima, cioè:

FileParser fileParser = ..........
FileParseResult result = fileParser.parse(file);

FileParser potrebbe essere una classe astratta così:

public abstract class FileParser {
    public final FileParseResult parse(File file) {
        // algoritmo ....
    }
}

Dentro parse c'è tutto l'algoritmo di parsing. Quello che è comune va lì dentro. La/e fase/i che invece devono/possono essere ridefinite in una sottoclasse vanno fatte con un metodo apposito (tipicamente protected) in modo che una sottoclasse deve/può ridefinirlo.

Dico in genere "deve/può" perché una fase potrebbe essere obbligatorio oppure opzionale ridefinirla.

per quanto riguarda il FileParserRegistry, non ho ben capito.
utilizza un altro pattern sul quale posso documentarmi?

No, non è un "pattern". Sarebbe una semplice classe che contiene un Map<String,FileParser> in cui puoi aggiungere le associazioni e poi poter estrarre un FileParser per il tipo. Tutto qua.

[fermat]

allora, sono un paio d'ore che ci sbatto la testa.
ma non ci arrivo, mi manca qualche pezzo.

allora, le parti comuni dovrebbero essere queste:

        // DA PASSARE AD APACHE TIKA
        BodyContentHandler handler = new BodyContentHandler();
        Metadata metadata = new Metadata();
        ParseContext context = new ParseContext();
        FileInputStream inputstream = new FileInputStream(file);
        // OUTPUT DA VISUALIZZARE NEI JPANEL
        StringBuilder sb;
        String[] metadataNames; // --> COSTRUITA DALLO STRINGBUILDER E VISUALIZZABILE NEL JPANEL DI SINISTRA
        String content; // --> VISUALIZZABILE NEL JPANEL DI DESTRA

in Apache Tika tutti i parser (sia quello generico AutoDetectParser che quelli specifici tipo OfficeParser) hanno il metodo parse al quale passare i primi 4 oggetti.
quindi riprendendo un esempio:

OfficeParser parser = new OfficeParser();
parser.parse(inputstream, handler, metadata, context);

poi ci stanno gli output da visualizzare nei JPanel.

quindi:
- quegli oggetti se li prende la classe che estende quella astratta, giusto?
- non ho ben capito quel FileParseResult in cosa consisterebbe

[andbin]

Scritto al volo, abbozzato, NON testato e SOLO per farti capire (in teoria dovrebbe funzionare):

public abstract class FileParser {
    public final FileParseResult parse(File file) throws IOException, SAXException, TikaException {
        BodyContentHandler handler = new BodyContentHandler();
        Metadata metadata = new Metadata();
        ParseContext context = new ParseContext();
        FileInputStream inputstream = new FileInputStream(file);

        Parser parser = createParser();
        parser.parse(inputstream, handler, metadata, context);

        String content = prepareContent(handler);

        return new FileParseResult(metadata.names(), content);
    }

    // Fase da ridefinire obbligatoriamente
    protected abstract Parser createParser();

    // Fase da ridefinire opzionalmente
    protected String prepareContent(BodyContentHandler handler) {
        return handler.toString();
    }
}


public class FileParseResult {
    private String[] metadataNames;
    private String content;

    // costruttore, getter/setter, ecc...
}



public class JPEGFileParser extends FileParser {
    protected Parser createParser() {
        return new JpegParser();
    }

    protected String prepareContent(BodyContentHandler handler) {
        return null;  // per JPEG è null
    }
}


public class PDFFileParser extends FileParser {
    protected Parser createParser() {
        return new PDFParser();
    }
}

[fermat]

ciao andbin!

intanto grazie mille!
ammetto che da solo non ci sarei arrivato, mi mancano troppi pezzi.

ho fatto qualche modifica per cercare di capire un pò meglio, e per cerecare di adattarlo.

public abstract class FileParser {

    public final FileParseResult parse(File file) throws IOException, SAXException, TikaException {
        BodyContentHandler handler = new BodyContentHandler();
        Metadata metadata = new Metadata();
        ParseContext context = new ParseContext();
        FileInputStream inputstream = new FileInputStream(file);
        Parser parser = createParser();
        parser.parse(inputstream, handler, metadata, context);
        return new FileParseResult(metadata, handler.toString());
    }

    protected abstract Parser createParser();
}

il prepareContent l'ho levato in quanto FileParseResult già mi da la stringa che mi serve.
però ho sostituito il primo parametro con Metadata, che elaboro dopo (ma ho qualce dubbio su quello che ho fatto).

public class FileParseResult {

    private Metadata metadataNames;
    private String content;

    public FileParseResult(Metadata metadataNames, String content) {
        this.metadataNames = metadataNames;
        this.content = content;
    }

    public Metadata getMetadataNames() {
        return metadataNames;
    }

    public void setMetadataNames(Metadata metadataNames) {
        this.metadataNames = metadataNames;
    }

    public String getContent() {
        return content;
    }

    public void setContent(String content) {
        this.content = content;
    }

}

questo un esempio di parser:

public class RTFFileParser extends FileParser {

    @Override
    protected Parser createParser() {
        return new RTFParser();
    }

}

e questo il registry:

public class FileParserRegistry {

    private Map<String, FileParser> map = new HashMap<>();

    public void add(FileParser parser, String type) {
        map.put(type, parser);
    }

    public void add(Parser parser, String type1, String type2) {
        
    }

    public FileParser getForType(String type) {
        return map.get(type);
    }
}

nella finestra:

            File f = new File(strFile);
            FileParserRegistry registry = new FileParserRegistry();
            registry.add(new RTFFileParser(), "application/rtf");
            FileParser parser = registry.getForType(strType);
            FileParseResult result = parser.parse(f);
            StringBuilder sb = new StringBuilder();
            for (String name : result.getMetadataNames().names()) {
                sb.append(name).append(": ").append(result.getMetadataNames().get(name)).append("\n");
            }
            WindowDetail wd = new WindowDetail(f, new PanelDetail(sb.toString()), new PanelDetail(result.getContent()));
            wd.setVisible(true);

quello su cui sono dubbioso sono due cose:
- l'elaborazione di Metadata, e quindi l'uso dello StringBuilder: lo metto qui, oppure forse è meglio elaborarlo direttamente nel PanelDetail?
- nella magior parte dei casi come secondo parametro di WindowDetail avrò un PanelDetail, a parte quando uso immagini o suoni: per evitare altri switch / if, potrei aggiungere anche quello al registry o devo seguire una strada ancora diversa?

[andbin]

il prepareContent l'ho levato in quanto FileParseResult già mi da la stringa che mi serve.

Ok, io avevo previsto quella fase semplicemente perché avevo visto che il tuo jpegParser() era l'unico che non faceva il setContent(handler.toString()); Quindi ho pensato: c'è per tutti (quindi è un "default") mentre per il jpeg no, quindi il content lo si potrebbe mettere fisso a null.

però ho sostituito il primo parametro con Metadata, che elaboro dopo (ma ho qualce dubbio su quello che ho fatto).

Puoi scegliere di mettere e tenere quello che vuoi nell'oggetto di "risultato". Insomma, metti pure quello che ritieni utile per chi userà il risultato!

e questo il registry:

public class FileParserRegistry {

    private Map<String, FileParser> map = new HashMap<>();

    public void add(FileParser parser, String type) {
        map.put(type, parser);
    }

    public void add(Parser parser, String type1, String type2) {
        
    }

    public FileParser getForType(String type) {
        return map.get(type);
    }
}

Ok, occhio solo che il secondo add ha Parser mentre invece dovrebbe essere FileParser. Io avevo pensato all'uso del varargs, ovvero un ultimo parametro String... types
Puoi metterne sia uno così, sia tenere anche le versioni in overload con 1 o 2 type (per comodità/efficienza). Assolutamente a tua scelta.

- l'elaborazione di Metadata, e quindi l'uso dello StringBuilder: lo metto qui, oppure forse è meglio elaborarlo direttamente nel PanelDetail?

Quella composizione di stringa non l'avevo messa nel FileParser perché in effetti il fatto di voler avere una forma aaa: bbb\nccc: ..... è una questione di "presentazione", riguarda la user interface ... non il parsing.

- nella magior parte dei casi come secondo parametro di WindowDetail avrò un PanelDetail, a parte quando uso immagini o suoni: per evitare altri switch / if, potrei aggiungere anche quello al registry o devo seguire una strada ancora diversa?

Se le possibilità sono poche es. testo o immagine o audio, questa informazione la potresti mettere nell'oggetto di result (e impostata da ciascuna classe di FileParser che ovviamente "sa" che tipo tratta) magari come enum. Poi fai uno switch. Non mi pare che sia una "brutta" cosa. Anche perché un conto è il parsing, un altro conto è la "presentazione" dei dati.
Ci sono sicuramente "design" migliori ma si dovrebbe andare un po' oltre e forse è troppo per il tuo caso o forse non serve.