Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How can I improve this method that adds data to three tables in the database?

The tables are:

  • UserTable
  • UserInfoTable
  • ContactTable

UserInfoTable and ContactTable have a one-to-many relationship, with ContactTable having UserInfoID as a foreign key.

public static boolean addUser(User user) {
Connection connection = getConnection();
try {
PreparedStatement insertUser = connection.prepareStatement(Insert_User);
insertUser.setString(1, user.getUsername()); 
insertUser.setString(2, user.getPassword());
insertUser.setString(3, user.getUserType());  
insertUser.executeUpdate(); 
PreparedStatement insertUserInfo = connection.prepareStatement(Insert_UserInfo);
insertUserInfo.setString(1, user.getFirstName()); 
insertUserInfo.setString(2, user.getMiddleName());
insertUserInfo.setString(3, user.getLastName());
insertUserInfo.setString(4, user.getGender());
insertUserInfo.setString(5, user.getBirthdate());
insertUserInfo.executeUpdate();
//I need to get the UserInfoID to insert to ContactTable. I really hate to do this but I can't think of any way to insert the foreign key.
PreparedStatement getUserInfoID = connection.prepareStatement(GET_USER_INFO_ID);
getUserInfoID.setString(1, user.getFirstName()); 
getUserInfoID.setString(2, user.getMiddleName());
getUserInfoID.setString(3, user.getLastName());
getUserInfoID.setString(4, user.getGender());
getUserInfoID.setString(5, user.getBirthdate());
ResultSet rs = getUserInfoID.executeQuery();
PreparedStatement insertContact = connection.prepareStatement(Insert_Contact);
while (rs.next()) {
insertContact.setInt(1, rs.getInt(1));
insertContact.setString(2, user.getEmailAddress()); 
insertContact.setString(3, user.getAddress());
insertContact.setString(4, user.getContactNumber());
insertContact.executeUpdate(); 
}
} catch (SQLException sqle) {
sqle.printStackTrace();
return false; 
}
return true;
}

I originally want to set the connections's autocommit to false so the connection will rollback if an insert statement fails, but since I need to get the UserInfo ResultSet to get the UserInfoID, I can't do it anymore. How can I do this better?

share|improve this question
    
Please post properly indented code so it's easier to read –  janos Jul 5 at 10:14

2 Answers 2

up vote 4 down vote accepted

janos recommended the Spring and Apache DbUtils frameworks, I can highly recommend using Hibernate.

Hibernate is a very commonly used ORM framework (Object-Relational-Mapping). By using Hibernate and the proper entity classes, you can do all this with only these lines:

try {
    em.getTransaction().begin();
    em.persist(user);
    em.getTransaction().commit();
} catch (Exception ex) {
    if (em.getTransaction().isActive()) {
        em.getTransaction().rollback();
    }
    // Log/handle exception in another way as well
}

When using Hibernate, you have the ability to tell it all about the one-to-many/many-to-many/one-to-one/many-to-one relationships, and tell it when to cascade operations for you. Here's an example of how your UserInfo could look.

@Entity
public class UserInfo {

    @Id
    @GeneratedValue(strategy = GenerationType.AUTO)
    private Long id;

    private String firstName;

    @ManyToOne(cascade = CascadeType.PERSIST)
    private List<Contact> contacts;

    // some getters and setters or other methods
}

When this entity gets persisted in Hibernate, it will also persist all it's contact information.

One formatting issue with your current code is that you're not indenting your code properly. Each { should add one indentation, each } should remove one. This makes things so much readable.

share|improve this answer
    
I will be using frameworks soon. –  recursivePointer Jul 5 at 13:35

I think you want to use transactions. Within a transaction, you can certainly do INSERT + INSERT + SELECT the UserInfoID + the final INSERT. Commit the transaction at the end, after all successful, rollback on any exception.

Rather than using the low level database APIs like managing Connection, PreparedStatement, ResultSet, yourself, I strongly recommend to use a framework for this, such as Spring's JDBC template or Apache's DbUtils. Once you step on that road life will get easier and you won't look back.

share|improve this answer
    
I will be using frameworks soon. –  recursivePointer Jul 5 at 13:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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