TrackDAO: what can be improved?

Question

In this specific situation there is a table with trackdata and a form to add rows. For the sake of clarity I won't include the view-related code here. The added rows are added to a LinkedList when appropriate.

public class TrackDAO {
private List<Track> tracks;
private Connection con;

public TrackDAO() {
    tracks = new LinkedList<Track>();
}

public void addTrack(Track track){
    tracks.add(track);
}

I use a Singleton class for the Database connection.

public void connect() throws Exception{
    con = Database.getInstance().connect();
}

public void disconnect() {
    Database.getInstance().disconnect();
}

Files can be stored & retrieved locally.

public void saveToFile(File file) throws IOException {
    File getFile = file;
    if (Utils.getExtension(getFile) == null){
        String url = getFile.getAbsolutePath();
        url += "." + Utils.ml;
        getFile = new File(url);
    }

    FileOutputStream fos = new FileOutputStream(getFile);
    ObjectOutputStream oos = new ObjectOutputStream(fos);

    Track[] trackArray = tracks.toArray(new Track[tracks.size()]);
    oos.writeObject(trackArray);
    oos.close();
}

public void loadFromFile(File file) throws IOException {
    FileInputStream fis = new FileInputStream(file);
    ObjectInputStream ois = new ObjectInputStream(fis);
    try {
        Track[] trackArray = (Track[])ois.readObject();
        tracks.clear();
        tracks.addAll(Arrays.asList(trackArray));
    } catch (ClassNotFoundException e) {
        e.printStackTrace();
    }
    ois.close();
}

When the table view is being opened the data will load into the table.

    public void loadData() throws SQLException{
    tracks.clear();
    String sql = "select id, artist, title, album, tuning, genre, url from track order by title";
    Statement selectStatement = con.createStatement();
    selectStatement.executeQuery(sql);
    ResultSet results = selectStatement.getResultSet();
        while(results.next()){
            int id = results.getInt("id");
            String artist = results.getString("artist");
            String title = results.getString("title");
            String album = results.getString("album");
            String tuning = results.getString("tuning");
            String genre = results.getString("genre");
            String fileUrl = results.getString("url");

            Track track = new Track(id, artist, title, album, tuning, genre, fileUrl);
            tracks.add(track);
            System.out.println(track);
        }
    results.close();
    selectStatement.close();
}

When the application is being closed the method will query the database for each id and check wether it has to update or insert the track.

public void saveToDatabase()throws SQLException {               
    String checkSql = "select count(*) as count from track where id=?";     
    PreparedStatement checkStatement = con.prepareStatement(checkSql);
    String insertSql = "insert into track(id, artist, title, album, tuning, genre, url) values(?,?,?,?,?,?,?)";
    PreparedStatement insertStatement = con.prepareStatement(insertSql);
    String updateSql = "update track set artist=?, title=?, album=?, tuning=?, genre=?, url=? where id=?";
    PreparedStatement updateStatement = con.prepareStatement(updateSql);


    for(Track track: tracks){
        int id = track.getId();
        String artist = track.getArtist();
        String title = track.getTitle();
        String album = track.getAlbum();
        String tuning = track.getTuning();
        String genre = track.getGenre();
        String url = track.getFileUrl();

        checkStatement.setInt(1, id);
        ResultSet checkResult = checkStatement.executeQuery();
        checkResult.next();
        int count = checkResult.getInt(1);

        if (count == 0){
            System.out.println("Inserting track with ID: " + id);
            int col = 1;
            insertStatement.setInt(col++, id);
            insertStatement.setString(col++, artist);
            insertStatement.setString(col++, title);
            insertStatement.setString(col++, album);
            insertStatement.setString(col++, tuning);
            insertStatement.setString(col++, genre);
            insertStatement.setString(col++, url);
            insertStatement.executeUpdate();
            System.out.println(insertStatement.toString());
        } else {
            System.out.println("Updating track with ID: " + id);
            int col = 1;
            updateStatement.setString(col++, artist);
            updateStatement.setString(col++, title);
            updateStatement.setString(col++, album);
            updateStatement.setString(col++, tuning);
            updateStatement.setString(col++, genre);
            updateStatement.setString(col++, url);
            updateStatement.setInt(col++, id);
            updateStatement.executeUpdate();
        }
    }
    updateStatement.close();
    insertStatement.close();
    checkStatement.close();
}

Finally a track can be deleted by id.

public void deleteTrack(int id) throws SQLException{
    String checkSql = "select count(*) as count from track where id=?";
    PreparedStatement checkStatement = con.prepareStatement(checkSql);
    checkStatement.setInt(1, id);
    ResultSet checkResult = checkStatement.executeQuery();
    checkResult.next();
    int count = checkResult.getInt(1);
    if(count!=0){
        for(Iterator<Track> it=tracks.iterator(); it.hasNext(); ) {
        if(it.next().getId()==id) { 
            System.out.println(it);
            it.remove();
            break;
            }
        }
        System.out.println("Deleting track with ID: " + id);
        String deleteSql = "delete from track where id=?"; 
        PreparedStatement deleteStatement = con.prepareStatement(deleteSql);
        deleteStatement.setInt(1, id);
        deleteStatement.executeUpdate();
        deleteStatement.close();
    }
    checkStatement.close();
}

Any advice (on either part or whole) is welcome.

Answer 1

• first of all, your DAO class is not thread safe. Are you sure this behavior is the goal?
• connect() method creates a connection every time when it is called, and doesn't disconnect the previous one, this could cause a memory leak
• at the same method throwing Exception is in order to avoid
• at saveToFile() why do you need a new pointer to the file object? It's code smell, and the original file is not declared as final so you should replace it anytime
• String url = getFile.getAbsolutePath();url += "." + Utils.ml; should be replace with String url getFile.getAbsolutePath() + "." + Utils.ml; It compiles to StringBuilder, so it's a cheaper operation then the 2 line version.
• at Track[] trackArray = tracks.toArray(new Track[tracks.size()]); you don't need to initialize the new array with the same size, 0 also will be ok
• instead of manually close resource(s) in Java 7 you should use an AutoCloseable with try
• loadFromFile() is not so nice. For me it's annoying that loadFromFile() drops all of my Tracks previously! This is strange, and need to know this behavior to use this class. If you want to write clean code, you have to avoid things like that.
• e.printStackTrace() is not acceptable in business applications, so if this is a hobby/school project it could be ok, otherwise it's a big mistake.
• loadData() -> tracks.clear() -> same story
• This class is not a clean DAO, it's a mixed something, because it not just representing Data Access, it contains other logic, and holding data. I suggest to split the class, create a thread safe stateless data access object, and a container which handles the current Tracks. In this way you should create arbitary number of containers, so you don't need to clear the tracks for example
• at saveToDatabase() check existing should be a separate method
• those Strings are code smell
• the if else contains code duplication, you do almost the same on different statements with the same class
• checkStatement.close() always close as soon as possible
• missing error handler, if some error occurs the statements stay open
• deleteTrack() code duplication!
• checkStatement.close() close ASAP