SQL , CSV Verbindung : Zu Umfangreicher Code durch Unwissenheit

[Mart]

deHallo,
In dieser Frage geht es nicht um einen Fehler sondern ich habe noch nicht das umfangreiche Wissen wie man Sachen macht bzw wie man Sachen aus gewissen Gründen NICHT machen sollte ich aber trotzdem mach weil ich das wissen dazu nicht habe, könnte ich Hinweise und meinungen zu dem Code bekommen den ich habe damit man den verbessern kann.
Der Code funktioniert...wahrscheinlich mehr schlecht als recht... xD
Der Code ist ein "Admin" Tool ...es wird und darf nur von meinen Leuten benutzt werden es wird nicht an den Client gegeben
Die java database Connector zeigt auf eine SQLite Datei die die Datenbank ist
Im Moment sind das alles Fixe Pfade auf meinem Laptop das wird dann später geändert je nachdem wo es dann benutzt wird

[CODE lang="java" title="SQLCode"]package sqltest;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Statement;

public class DataBaseController{
private static Connection connection = null;
private static Statement statement;
private static final String csvDelimiter = ",";
private String CreatureCardCsvPath = "/home/legacy/Dokumente/CardDatabaseCsv.csv";
private static String currentPath ="",line = "";
private static DataBaseController db = new DataBaseController();
public static void main(String[] args) throws Exception{
try{
connection = DriverManager.getConnection("jdbc:sqlite:/home/legacy/Bilder/CardDatabse.db");
statement = connection.createStatement();
statement.setQueryTimeout(30);
db.startInsertionOfCsv(db.CreatureCardCsvPath);
}
catch(SQLException e){
System.err.println(e.getMessage());
}
finally {
closesConnection();
}
}
public static void closesConnection(){
try{
if(connection != null) connection.close();
}
catch(SQLException e){
System.err.println(e.getMessage());
}
}

public void startInsertionOfCsv(String path) throws IOException, NoSuchMethodException, SecurityException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
currentPath = path;
DataBaseController k=new DataBaseController();
String text = "creatureInsertion";
Class<? extends DataBaseController> c = getClass();
Method m=c.getDeclaredMethod(text);
m.invoke(k);
}
public static void creatureInsertion() throws Exception {
BufferedReader br = null;
br = new BufferedReader(new FileReader(currentPath));
line = "";
while ((line = br.readLine()) != null) {
String[] cardValueLine = line.split(csvDelimiter);
PreparedStatement ps = connection.prepareStatement(
"INSERT INTO CreatureCards ( Name,Image ,Effect, HealthPoints , Strength , Level, Artist ) VALUES (?,?,?,?,?,?,?) ");
ps.setString(1, cardValueLine[0]);
File file = new File("/home/legacy/CardImages/"+cardValueLine[1]+".png");
FileInputStream fis = new FileInputStream(file);
ps.setBinaryStream(2, fis,(int)file.length());
ps.setString(3, cardValueLine[2].replace("\\n",System.lineSeparator()));
ps.setInt(4,Integer.parseInt(cardValueLine[3]));
ps.setInt(5,Integer.parseInt(cardValueLine[4]));
ps.setInt(6,Integer.parseInt(cardValueLine[5]));
ps.setString(7,cardValueLine[6]);
ps.executeUpdate();
ps.close();
}
br.close();
}
}[/CODE]

 

[temi]

Erst mal solltest du deinen Code soweit anpassen, dass das einzige "static" vor der Methode main() steht.

Warum rufst du denn die Methode creatureInsertion() nicht direkt auf, sondern über den Umweg per Reflection?

 

[Mart]

Erst mal solltest du deinen Code soweit anpassen, dass das einzige "static" vor der Methode main() steht.

Warum rufst du denn die Methode creatureInsertion() nicht direkt auf, sondern über den Umweg per Reflection?

es wird 3 verschiedene insertion methoden geben , wäre es sinnvoll da das dann umzubauen um nur normale methoden zu haben ?

 

[temi]

es wird 3 verschiedene insertion methoden geben , wäre es sinnvoll da das dann umzubauen um nur normale methoden zu haben ?

Ich kenne die Unterschiede deiner drei Methoden nicht, aber vermutlich ist Reflection eine der schlechtesten Methoden dies umzusetzen.

 

[temi]

Weiterhin würde sich den Code etwas mehr in seine Aufgaben zerlegen, also eine Klasse Creature mit ihren Eigenschaften und ein Klasse CsvReader, welche die Daten aus der Datei liest und in eine Collection<Creature> speichert. Diese Collection kannst du dann hernehmen, um die einzelnen Creatures in die DB einzufügen "insertCreature(Creature creature)". Jede Klasse sollte nur eine Aufgabe haben.

 

[mrBrown]

  public void startInsertionOfCsv(String path) throws IOException, NoSuchMethodException, SecurityException, IllegalAccessException, IllegalArgumentException, InvocationTargetException {
      currentPath = path;
      DataBaseController k=new DataBaseController();
      String text = "creatureInsertion";
      Class<? extends DataBaseController> c = getClass();
      Method m=c.getDeclaredMethod(text);
      m.invoke(k);
  }

Ich habe keine Ahnung, was du dir dabei gedacht hast, aber der "einfache" Weg wäre:

  public void startInsertionOfCsv(String path) throws Exception {
      currentPath = path;
      DataBaseController.creatureInsertion();
  }

Ansonsten (erstmal ohne das Design selbst zu beurteilen):

* Für alles, was ein close braucht, try-with-resources benutzen
* Den fis in Zeile 66 schließt du nicht
* Zum Einfügen der Einträge kann man batch processing nutzen,
* System.lineSeparator() sollte man recht selten nutzen müssen, intern (und dazu gehört die DB) sollte man besser immer mit \n arbeiten.
* throws Exception, solltest du nie benutzen, immer die konkrete Exception angeben

 

[Mart]

package sqltest;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Statement;

public class LauncherMain{
    public static void main(String[] args) throws Exception{
        DataBaseController dbc = new DataBaseController();
    }
}
class DataBaseController{
    private Connection connection = null;
    private Statement statement;
    private final String csvDelimiter = ",";
    private String CreatureCardCsvPath = "/home/legacy/Dokumente/CardDatabaseCsv.csv";
    private String currentPath ="",line = "";

    public DataBaseController() throws Exception {
           try{
                connection = DriverManager.getConnection("jdbc:sqlite:/home/legacy/Bilder/CardDatabse.db");
                statement = connection.createStatement();
                statement.setQueryTimeout(30);
                this.startInsertionOfCsv(this.CreatureCardCsvPath);
            }
            catch(SQLException e){
              System.err.println(e.getMessage());
            }   
            finally {
                closesConnection();
            }
    }
private void closesConnection(){
    try{
      if(connection != null) connection.close();
    }
    catch(SQLException e){
      System.err.println(e.getMessage());
    }
}

private void startInsertionOfCsv(String path) throws Exception {
    currentPath = path;
    creatureInsertion();
}
private void creatureInsertion() throws Exception {
    BufferedReader br = null;
    br = new BufferedReader(new FileReader(currentPath));
    this.line = "";
    while ((line = br.readLine()) != null) {
        String[] cardValueLine = line.split(csvDelimiter);
        PreparedStatement ps = connection.prepareStatement(
                "INSERT INTO CreatureCards ( Name,Image ,Effect, HealthPoints , Strength , Level, Artist ) VALUES (?,?,?,?,?,?,?) ");
        ps.setString(1, cardValueLine[0]);
        File file = new File("/home/legacy/CardImages/"+cardValueLine[1]+".png");
        FileInputStream fis = new FileInputStream(file);
        ps.setBinaryStream(2, fis,(int)file.length());
        ps.setString(3, cardValueLine[2].replace("\\n",System.lineSeparator()));
        ps.setInt(4,Integer.parseInt(cardValueLine[3]));
        ps.setInt(5,Integer.parseInt(cardValueLine[4]));
        ps.setInt(6,Integer.parseInt(cardValueLine[5]));
        ps.setString(7,cardValueLine[6]);
        ps.executeUpdate();
        ps.close();
    }
    br.close();
    }
}

Das ist der Überarbeitete code.

Was hat es mit dem "nichts static" machen auf sich warum sollte man das nicht tun?

 

[mrBrown]

es wird 3 verschiedene insertion methoden geben , wäre es sinnvoll da das dann umzubauen um nur normale methoden zu haben ?

Ja, auf jeden Fall.

Der Nutzer soll nicht den konkreten Methodennamen irgendwo auswählen müssen, die Aufrufe sind aktuell ohne Compiler-Prüfung, und die Methoden werden aktuell von jedem statischem Check als unbenutzt erkannt – alles drei willst du nicht. Selbst ein einfaches switch über die Nutzereingabe zum Auswählen der Methode wäre da deutlich besser.

 

[temi]

Was hat es mit dem "nichts static" machen auf sich warum sollte man das nicht tun?

Weißt du denn was das "static" bewirkt?

 

[mrBrown]

Das ist der Überarbeitete code.

Ein Konstruktor sollte im Idealfall nur das Objekt in einen gültigen Zustand bringen, aber nicht die gesamte Logik ausführen, eine extra Methode zum Starten ist meist sinnvoller. Ebenso macht es sinn, dass wen ein Objekt einen Zustand hält, der geschlossen werden muss, dass explizit zu machen:

Ganz grob etwa:

class DataBaseController implements AutoCloseable, Runnable {
    ...
}

...

public static void main(String[] args) throws Exception{
    try (DataBaseController dbc = new DataBaseController()) {
        doc.run();
    }
}

Was hat es mit dem "nichts static" machen auf sich warum sollte man das nicht tun?

static macht es erstmal auf den ersten Blick "einfacher", fällt einem später aber immer auf die Füße. Ein Beispiel dafür sind Tests, mit static musst du aufpassen, dass ein Test den nächsten nicht beeinflusst – ohne static gibt es das Problem nicht.

 

[Mart]

Weißt du denn was das "static" bewirkt?

dass es das Objekt überall dann erreichbar ist im programm mehr nicht

 

[Mart]

Ein Konstruktor sollte im Idealfall nur das Objekt in einen gültigen Zustand bringen, aber nicht die gesamte Logik ausführen, eine extra Methode zum Starten ist meist sinnvoller. Ebenso macht es sinn, dass wen ein Objekt einen Zustand hält, der geschlossen werden muss, dass explizit zu machen:

Ganz grob etwa:

class DataBaseController implements AutoCloseable, Runnable {
    ...
}

...

public static void main(String[] args) throws Exception{
    try (DataBaseController dbc = new DataBaseController()) {
        doc.run();
    }
}

static macht es erstmal auf den ersten Blick "einfacher", fällt einem später aber immer auf die Füße. Ein Beispiel dafür sind Tests, mit static musst du aufpassen, dass ein Test den nächsten nicht beeinflusst – ohne static gibt es das Problem nicht.

ich habe jetzt alles gemacht was du gesagt hast außer das mit dem runnable weil das programm wird sehr selten ausgeführt und hat so viel zeit wie es will (bitte nicht negativ auffassen 😀)


[CODE lang="java" title="Verbesserter Code"]package sqltest;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStream;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Statement;

public class LauncherMain{
public static void main(String[] args) throws Exception{
DataBaseController dbc = new DataBaseController();
dbc.runCreatureInsert();
dbc.stopInsertion();
}
}
class DataBaseController{
private Connection connection = null;
private Statement statement;
private final String csvDelimiter = ",";
private String CreatureCardCsvPath = "/home/legacy/Dokumente/CardDatabaseCsv.csv";
private String line = "";

public DataBaseController() {
try{
connection = DriverManager.getConnection("jdbc:sqlite:/home/legacy/Bilder/CardDatabse.db");
statement = connection.createStatement();
statement.setQueryTimeout(30);
}
catch(SQLException e){
System.err.println(e.getMessage());
}
}
private void closesConnection() throws SQLException{
if(connection != null) connection.close();
}

private void creatureInsertion(String currentPath) throws NumberFormatException, IOException, SQLException {
BufferedReader br = null;
br = new BufferedReader(new FileReader(currentPath));
line = "";
PreparedStatement ps = connection.prepareStatement(
"INSERT INTO CreatureCards ( Name,Image ,Effect, HealthPoints , Strength , Level, Artist ) VALUES (?,?,?,?,?,?,?) ");

while ((line = br.readLine()) != null) {
String[] cardValueLine = line.split(csvDelimiter);
ps.setString(1, cardValueLine[0]);
File file = new File("/home/legacy/CardImages/"+cardValueLine[1]+".png");
FileInputStream fis = new FileInputStream(file);
ps.setBinaryStream(2, fis,(int)file.length());
ps.setString(3, cardValueLine[2].replace("\\n","\n"));
ps.setInt(4,Integer.parseInt(cardValueLine[3]));
ps.setInt(5,Integer.parseInt(cardValueLine[4]));
ps.setInt(6,Integer.parseInt(cardValueLine[5]));
ps.setString(7,cardValueLine[6]);
ps.addBatch();
fis.close();
}
ps.executeBatch();
br.close();
ps.close();

}
public void runCreatureInsert() throws NumberFormatException, IOException, SQLException{
this.creatureInsertion(this.CreatureCardCsvPath);
}
public void stopInsertion() {
try {
closesConnection();
}
catch (SQLException e) {
e.printStackTrace();
}
}
}[/CODE]
Anmerkung: Es wird dazu keine GUI geben man startet es und wartet bis es fertig ist

 

[Mart]

w

 

[Mart]

Weißt du denn was das "static" bewirkt?

wie sieht es jetzt mit dem code aus

 

[mrBrown]

außer das mit dem runnable weil das programm wird sehr selten ausgeführt und hat so viel zeit wie es will (bitte nicht negativ auffassen 😀)

Runnable hat nichts mit der Dauer oder so, es gibt dann einfach nur eine Methode „run“, mehr nicht.
Das kann man dann auch nutzen, wenn man es mit Threads beschleunigen will, muss man aber nicht.

wie sieht es jetzt mit dem code aus

try-with-resources sollte für alles genutzt werden und das Statement in Zeile 36 dürfte überflüssig sein.
Variablen sollten immer einen möglichst kleinen Scooe haben, das betrifft zB line, die nur in einer Methode benutzt wird, aber als Instanzvariable deklariert ist.

 

[Mart]

Runnable hat nichts mit der Dauer oder so, es gibt dann einfach nur eine Methode „run“, mehr nicht.
Das kann man dann auch nutzen, wenn man es mit Threads beschleunigen will, muss man aber nicht.



try-with-resources sollte für alles genutzt werden und das Statement in Zeile 36 dürfte überflüssig sein.
Variablen sollten immer einen möglichst kleinen Scooe haben, das betrifft zB line, die nur in einer Methode benutzt wird, aber als Instanzvariable deklariert ist.

vielen dank dann übernehm ich das auch noch 😀

 

[mihe7]

dass es das Objekt überall dann erreichbar ist im programm mehr nicht

Mit static deklarierst Du Attribute und Methoden, die sich auf die Klasse in ihrer Gesamtheit und nicht auf Objekte dieser Klasse beziehen. Damit gibt es auch keine Polymorphie und in Verbindung mit public führtst Du ggf. globale Variablen ein.

Problem: von überall aus kann auf alles zugegriffen werden. Das ist die beste Voraussetzung für Spaghetti-Code und Effekte, die man nicht will.

Beispiel: ich hab mal eine App übernommen, da wurde alles mögliche global erledigt. An einer Stelle eine Zeile rausgeworfen, plötzlich funktionierte an ganz anderen Stellen die App nicht mehr richtig. Ich habe dann zwei Wochen damit zugebracht, den Code zu entwirren und in eine halbwegs(!) vernünftige Form zu bringen...

 

[Mart]

Habe jetzt alles geändert was ihr erwähnt hattet auch das mit run und das mit dem try with res da steig ich noch nicht durch ...aber mit der zeit solls schon werden xD Vielen dank soweit

 

[Mart]

gibt es noch etwas was man überarbeiten könnte? es ist wichtig dass der code so gut wie möglccih ist ansonsten schneid ich mich selber xD

 

[Mart]

[CODE lang="java" title="Jetziger Zustand"]package sqltest;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStream;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Statement;

public class LauncherMain {
public static void main(String[] args) throws SQLException {
DataBaseController dbc = new DataBaseController();
try {
dbc.run();
dbc.close();
}
catch(Exception e) {
e.getMessage();
}
}
}
class DataBaseController implements AutoCloseable,Runnable{
private Connection connection = null;
private final String csvDelimiter = ",";
private String CreatureCardCsvPath = "/home/legacy/Dokumente/CardDatabaseCsv.csv";
private String SorceryCardCsvPath = "home/legacy/Dokumente/SorceryDatabaseCsv.csv";
private String CommanderCardCsvPath = "home/legacy/Dokumente/SorceryDatabaseCsv.csv";
private String line = "";

public DataBaseController() throws SQLException {
try {
connection = DriverManager.getConnection("jdbc:sqlite:/home/legacy/Bilder/CardDatabse.db");
}
catch(SQLException e) {
System.err.println(e.getMessage());
}
PreparedStatement ps = connection.prepareStatement("CREATE TABLE CommanderCards ( Name VARCHAR PRIMARY KEY,Image BLOB,HealthPoints VARCHAR, Effect VARCHAR, Artist VARCHAR)");
ps.execute();
}
private void closesConnection() throws SQLException {
if(connection != null) connection.close();
}

private void creatureInsertion(String currentPath) throws NumberFormatException, IOException, SQLException {
BufferedReader br = new BufferedReader(new FileReader(currentPath));
line = "";
PreparedStatement ps = connection.prepareStatement(
"INSERT INTO CreatureCards ( Name,Image ,Effect, HealthPoints , Strength , Level, Artist ) VALUES (?,?,?,?,?,?,?) ");

while ((line = br.readLine()) != null) {
String[] cardValueLine = line.split(csvDelimiter);
ps.setString(1, cardValueLine[0]);
File file = new File("/home/legacy/CardImages/"+cardValueLine[1]+".png");
FileInputStream fis = new FileInputStream(file);
ps.setBinaryStream(2, fis,(int)file.length());
ps.setString(3, cardValueLine[2].replace("\\n","\n"));
ps.setInt(4,Integer.parseInt(cardValueLine[3]));
ps.setInt(5,Integer.parseInt(cardValueLine[4]));
ps.setInt(6,Integer.parseInt(cardValueLine[5]));
ps.setString(7,cardValueLine[6]);
ps.addBatch();
fis.close();
}
ps.executeBatch();
br.close();
ps.close();
}

public void runCreatureInsert() throws NumberFormatException, IOException, SQLException {
this.creatureInsertion(this.CreatureCardCsvPath);
}
public void runSorceryInsert() throws NumberFormatException, IOException, SQLException {
this.sorceryInsertion(this.SorceryCardCsvPath);
}
public void runCommanderInsert() throws NumberFormatException, IOException, SQLException {
this.commanderInsertion(this.CommanderCardCsvPath);
}
public void close() throws Exception {
try {
closesConnection();
}
catch (SQLException e) {
e.printStackTrace();
}
}
public void run() {
try {
runCreatureInsert();

} catch (Exception e) {
e.printStackTrace();
}

}
}

[/CODE]

 

[temi]

gibt es noch etwas was man überarbeiten könnte? es ist wichtig dass der code so gut wie möglccih ist ansonsten schneid ich mich selber xD

Schau dir nochmal #6 und #10 bezüglich try-with-resources an.

// nicht
DataBaseController dbc = new DataBaseController();
try {
    dbc.run();
    dbc.close();
}

// sondern
try (DataBaseController dbc = new DataBaseController()) {
    dbc.run();
    // der explizite Aufruf von close() ist nicht notwendig, dafür ja try-with-resources
}

 

[Mart]

Schau dir nochmal #6 und #10 bezüglich try-with-resources an.

// nicht
DataBaseController dbc = new DataBaseController();
try {
    dbc.run();
    dbc.close();
}

// sondern
try (DataBaseController dbc = new DataBaseController()) {
    dbc.run();
    // der explizite Aufruf von close() ist nicht notwendig, dafür ja try-with-resources
}

ich habe das jetzt mal versucht zu implementieren bin aber teilweise gescheitert ich hätte es jetzt so umgebaut

    public void run() {
        try (BufferedReader br = new BufferedReader(new FileReader(this.CreatureCardCsvPath))) {
            creatureInsertion(br);
        } catch (Exception e) {
            e.printStackTrace();
        }   
    }


    private void creatureInsertion(BufferedReader br) throws NumberFormatException, IOException, SQLException {
        line = "";
        PreparedStatement ps = connection.prepareStatement(
                  "INSERT INTO CreatureCards ( Name,Image ,Effect, HealthPoints , Strength , Level, Artist ) VALUES (?,?,?,?,?,?,?) ");
    
        while ((line = br.readLine()) != null) {
                String[] cardValueLine = line.split(csvDelimiter);
                 ps.setString(1, cardValueLine[0]);
            File file = new File("/home/legacy/CardImages/"+cardValueLine[1]+".png");
            FileInputStream fis = new FileInputStream(file);
            ps.setBinaryStream(2, fis,(int)file.length());
            ps.setString(3, cardValueLine[2].replace("\\n","\n"));
            ps.setInt(4,Integer.parseInt(cardValueLine[3]));
            ps.setInt(5,Integer.parseInt(cardValueLine[4]));
            ps.setInt(6,Integer.parseInt(cardValueLine[5]));
            ps.setString(7,cardValueLine[6]);
            ps.addBatch();
            fis.close();
        }
        ps.executeBatch();
        br.close();
        ps.close();
    }

sieht das jetzt besser aus? wie ich das mit dem try catch schon in der main methode mache versteh ich leider nicht

hätte jemand da einen link wo das erklärt wird wie das über 3 methoden funktioniert