1
\$\begingroup\$

The application is a student administration system using a .csv file for persistence as part of our school project.

This is my first created a basic DAO. I'd like to have some feedback about what could be improved until now and how I could write good unit tests for the UserDAO.class.

UserDAO.java

package com.studentenverwaltung.persistence;

import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;

import org.supercsv.cellprocessor.constraint.NotNull;
import org.supercsv.cellprocessor.constraint.Unique;
import org.supercsv.cellprocessor.ift.CellProcessor;
import org.supercsv.io.CsvBeanReader;
import org.supercsv.io.CsvBeanWriter;
import org.supercsv.io.ICsvBeanReader;
import org.supercsv.io.ICsvBeanWriter;
import org.supercsv.prefs.CsvPreference;

import com.studentenverwaltung.model.User;

/**
 * 
 * @author philippkosel
 * 
 */
public class UserDAO {
    private static final String CSV_FILE = "/com/studentenverwaltung/resources/test.csv";

    private ArrayList<User> users;
    private URL url;

    /**
     * 
     */
    public UserDAO() {
        this.users = new ArrayList<User>();
        this.url = getClass().getResource(this.CSV_FILE);
    }

    /**
     * 
     * @return
     */
    private static CellProcessor[] getProcessors() {
        final CellProcessor[] processors = new CellProcessor[] { new Unique(), // id
                new NotNull() // password
        };

        return processors;
    }

    /**
     * 
     * @throws IOException
     */
    private void readCSV() throws IOException {
        ICsvBeanReader beanReader = null;

        try {
            beanReader = new CsvBeanReader(new FileReader(this.url.getPath()),
                    CsvPreference.STANDARD_PREFERENCE);

            final String[] header = beanReader.getHeader(true);
            final CellProcessor[] processors = getProcessors();

            User newUser;

            while ((newUser = beanReader.read(User.class, header, processors)) != null) {
                this.users.add(newUser);
            }
        } finally {
            if (beanReader != null) {
                beanReader.close();
            }
        }
    }

    /**
     * 
     * @throws IOException
     */
    private void writeCSV() throws IOException {
        ICsvBeanWriter beanWriter = null;

        try {
            File temp = File.createTempFile("tempfile", ".tmp");

            beanWriter = new CsvBeanWriter(new FileWriter(temp),
                    CsvPreference.STANDARD_PREFERENCE);

            final String[] header = new String[] { "id", "password" };
            final CellProcessor[] processors = getProcessors();

            beanWriter.writeHeader(header);

            for (User user : this.users) {
                beanWriter.write(user, header, processors);
            }

            File file = new File(this.url.getPath());
            file.delete();

            temp.renameTo(file);
        } finally {
            if (beanWriter != null) {
                beanWriter.close();
            }
        }

    }

    /**
     * 
     * @param user
     * @throws IOException
     */
    public void add(User user) throws IOException {
        readCSV();
        this.users.add(user);
        writeCSV();
    }

    /**
     * 
     * @param user
     * @throws IOException
     */
    public void update(User user) throws IOException {
        readCSV();

        for (User userToBeUpdated : this.users) {
            if (userToBeUpdated.getId().equals(user.getId())) {
                this.users.remove(userToBeUpdated);
                this.users.add(user);
            }
        }

        writeCSV();
    }

    /**
     * 
     * @param userId
     * @throws IOException
     */
    public void delete(String userId) throws IOException {
        readCSV();

        for (User userToBeDeleted : this.users) {
            if (userToBeDeleted.getId().equals(userId)) {
                this.users.remove(userToBeDeleted);
            }
        }

        writeCSV();
    }

    /**
     * 
     * @return
     * @throws IOException
     */
    public List<User> findAll() throws IOException {
        readCSV();

        return this.users;
    }

    /**
     * 
     * @param userId
     * @return
     * @throws IOException
     */
    public User findByPrimaryKey(String userId) throws IOException {
        readCSV();

        for (User userToBeReturned : this.users) {
            if (userToBeReturned.getId().equals(userId)) {
                return userToBeReturned;
            }
        }

        return null;
    }
}

UserDAOTest.java

package com.studentenverwaltung.persistence;

import org.junit.Test;

import com.studentenverwaltung.model.User;

public class UserDAOTest {

    @Test
    public void testAdd() {
        UserDAO ud = new UserDAO();

        User user = new User();
        user.setId("test");
        user.setPassword("test");

        ud.add(user);
    }
}
\$\endgroup\$
4
\$\begingroup\$

It looks like there is a potential bug in writeCSV(). You open a temp file and write to it, but then you move that file before closing it.

try {
    File temp = File.createTempFile("tempfile", ".tmp");
    beanWriter = new CsvBeanWriter(new FileWriter(temp), CsvPreference.STANDARD_PREFERENCE);
    // ...
    temp.renameTo(file); // <-- moving a file that hasn't been closed yet!
} finally {
    if (beanWriter != null) {
        beanWriter.close();
    }
}

You are currently accessing the file as a classpath resource:

getClass().getResource(this.CSV_FILE);

While this may work in development, it is unlikely to work when bundling the application into a jar and/or into a larger application. The file will likely reside inside the jar, which could prevent reading/writing it as a File. I would recommend using a different strategy for storing & locating the file.


CSV_FILE is declared as a static member. This means that it belongs to the class itself, not to any particular instance of the class. You are accessing it in a non-static way. It is more common to access it in a static way, like one of these:

this.url = getClass().getResource(CSV_FILE);
this.url = getClass().getResource(UserDAO.CSV_FILE);

Since it is private (and therefore, only referenced from within this class), I tend to prefer the first one.


The use of the users member seems odd to me. It is not used to maintain state beyond any single method. I don't see any compelling reason for it to be a member of the class. Consider changing readCSV() and writeCSV() to return and accept, respectively, a Collection of User, and localize users to the methods that need it.

private Collection<User> readCSV() throws IOException { ... }
private void writeCSV(Collection<User> users) throws IOException { ... }

The other methods can do something like this:

public void add(User user) throws IOException {
    Collection<User> users = readCSV();
    users.add(user);
    writeCSV(users);
}

This makes it a bit easier to read these methods by limiting the scope of users. There is no longer any state to keep track of outside the scope of the method.


Notice I am also using Collection instead of List or ArrayList. The use of List implies to me that the users are in some defined order. I don't see anything in this code that enforces any specific order, so I chose to use Collection instead. You should use the appropriate collection type to reveal meaning/intent. If order matters and is meaningful, stick with List. If order does not matter, use Collection. Set is also an option. A Set would indicate that order does not matter, but uniqueness does.

\$\endgroup\$
  • \$\begingroup\$ Hey Joe, thank you for your detailed answer! So HashSet would a good option to use if order doesn't matter? \$\endgroup\$ – philk982 May 4 '13 at 21:10
  • \$\begingroup\$ A HashSet would be fine if order does not matter. Being an implementation of Set, it also guarantees and enforces that no duplicate elements exist. The documentation of each collection type will typically define its behavioral and performance characteristics. Also, the specific collection class used should be a private implementation detail. The method declarations should still use something like Collection or Set. \$\endgroup\$ – Joe F May 5 '13 at 0:38
  • \$\begingroup\$ Hey Joe, tried to implement your feedback. Please could you review my basic MVC archticeture/login view and give me some more feedback...? ;-) github.com/pixelpurist/studentenverwaltung Is it ok to call the DAO from within the view or should it be accessed elsewhere? Thank you Philipp \$\endgroup\$ – philk982 May 5 '13 at 15:54
  • \$\begingroup\$ It is probably best to start a new question with the new code to be reviewed. If relevant, you may also want include a little background and maybe a link to this question. \$\endgroup\$ – Joe F May 5 '13 at 17:11
0
\$\begingroup\$

sorry for spelling and grammar i'm on a tablet.

As for your unit tests you should always do your tests first. think of it like a school test. does your teacher give you a blank paper and tell you you have thirty minutes to finish. your unit tests are the questions on a test. your production code is the answer. asserts is your teacher checking your answers.

so with your tests think to your self, how do i want to save my users, how do i want to load my users into memory, how should i add more users to my collection...etc once you get those questions written then write your code to do that. now in the future if you forget how a method works you can just go back to your unit test and see how you implemented it. i hope this illustration gives you a small step in the right direction

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.