Oh certo ... ma l'elenco è lungo (e non sarò sicuramente esaustivo al 100%):Originariamente inviata da linux_r
- il codice non è completo ma dai metodi si deduce che SWListaOperatori è uno SwingWorker. SWListaOperatori contiene una inner-class SWInviaAppuntamento che è di nuovo uno SwingWorker (di per sé tecnicamente nulla di sbagliato ma è perlomeno strano/inusuale). Nel doInBackground di SWInviaAppuntamento c'è una anonymous inner class per un Runnable. Nel suo run() ad un certo punto c'è una ulteriore anonymous inner-class per un WindowAdapter. Quando si arriva ad un tale livello di annidamento con pure tutti i vari blocchi if/try in mezzo, dovrebbe suonarti un campanello di allarme .... un grosso campanello ...
- in questo codice usi di tutto e di più: da SwingWorker, ai thread, alla sincronizzazione con synchronized, all'uso di volatile, all'uso della condition-queue "intrinseca" (il wait/notify sugli oggetti). Dubito che quello sia l'unico modo per fare quello che volevi ma anche se fosse davvero tutto necessario, così è comunque un po' sparpagliato e fumoso.
- nel primo doInBackground non si capisce assolutamente che cosa si ottiene. Cosa conterrà quel ArrayList<Object>? Che colonna (e di che tipo) prendi con getObject(1) ? Solo tu lo sai ....
Manca di chiarezza.
- sempre in questo doInBackground il close non viene fatto sempre. Se next() o getObject() lanciano SQLException, tu non chiudi nulla. Manca di correttezza nella gestione delle eccezioni.
E anche se facessi tutto corretto con le eccezioni, tutta la logica di query ed estrazione dal result-set andrebbe fatta in una classe separata.
- guardando le invocazioni sulla istanza di Database, sembrano un po' una accozzaglia di operazioni ben diverse. Evita di fare una classe "Database" che fa di tutto e di più. Avresti potuto/dovuto applicare almeno il pattern DAO, per risolvere anche quanto detto al punto precedente.
- Database.getInstance().updateField(10, 1, Integer.toString(cliente.getId_cliente()), (String) userOperatore, GestoreDatabase.Tabella.CLIENTE);
Non è un buon modo di fare un update di uno o più campi. Tra l'altro "sembra" un metodo molto generico/arbitrario e quindi poco comprensibile. Cosa sono ad esempio 10 e 1 ? Solo tu lo sai ....
- nel run() del tuo thread vai ad accedere alla interfaccia utente. Non sei nel contesto del EDT, tutto quello che fai con JOptionPane, setVisible(true) ecc... è tutto inappropriato.
- nel run() comunque c'è un mix di accesso a database, interfaccia grafica, wait/notify. Tutto potenzialmente molto dubbio (fumoso sicuramente).
- fai un synchronized su schedaCliente che è un oggetto GUI. Per quanto ne so, non c'è nulla di intrinsecamente (e tecnicamente) errato nell'usare oggetti GUI come "lock". Ma personalmente io non lo farei mai nemmeno se mi pagate il doppio del mio stipendio ....
E mi sono limitato solo alle cose più evidenti ....
Rispondi quotando