Displaying user scores

Question

I have started teaching myself MySQL and created a simple application which asks for user input stores it and displays it. Can you guys please let me know if there is something I am doing wrong or any tips on what can be improved? I want to build a student management application which will use a MySQL database and want to make sure I know the basics.

I have a class that holds connection method and closing methods:

import java.sql.*;
public class DBUtil {
// Connecting to database.
public static Connection getConnection() throws SQLException {
String url = "jdbc:mysql://localhost:3306/database";
String user = "root";
String pass = "admin";
Connection connection = DriverManager.getConnection(url, user, pass);
 return connection;
}
// Close database connection.
public static void closeConnection(Connection con) {
 try {
 if(con != null) {
 con.close();
 System.out.println("Database Connection Closed!");
 }
 }catch(Exception e) {
 e.printStackTrace();
 }
}
// Close Prepared Statement.
public static void closePrepStatement(PreparedStatement pStmt) {
 try {
 if(pStmt != null) {
 pStmt.close();
 System.out.println("Prepared Statemet Closed");
 }
 }catch (Exception e) {
 e.printStackTrace();
 }
}
// Close Result Set.
public static void closeResSet(ResultSet rSet) {
 try{
 if(rSet != null) {
 rSet.close();
 System.out.println("Result Set Closed");
 }
 }catch (Exception e) {
 e.printStackTrace();
 }
}
}

This class gets the input, stores it, and displays it:

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.util.Scanner;
// SQL variables defined
public class Source {
DBUtil dbConnection = new DBUtil();
Scanner input = new Scanner(System.in);
Connection connection = null;
PreparedStatement pStatement = null;
ResultSet rSet = null;
// Constructor
public Source() {
 getInput();
 displayData();
}
// Getting user input and saving to database
private void getInput() {
 System.out.println("What is your name?");
 String fName = input.nextLine();
 System.out.println("What is your second name?");
 String sName = input.nextLine();
 System.out.println("What is your proffesion?");
 String userProff = input.nextLine();
 try {
 connection = DBUtil.getConnection();
 String sqlQuery = "INSERT INTO employees_info (firstname, secondname, proffesion) VALUES (?, ?, ?)";
 pStatement = connection.prepareStatement(sqlQuery);
 pStatement.setString(1, fName);
 pStatement.setString(2, sName);
 pStatement.setString(3, userProff);
 pStatement.executeUpdate();
 System.out.println("Record saved!");
 }catch(Exception e) {
 e.printStackTrace();
 }finally {
 DBUtil.closePrepStatement(pStatement);
 DBUtil.closeConnection(connection);
 }
}
// Get data from database and display
private void displayData() {
 System.out.println("Current records from the database:");
 try {
 connection = DBUtil.getConnection();
 String sqlQuery = "SELECT * FROM employees_info";
 pStatement = connection.prepareStatement(sqlQuery);
 rSet = pStatement.executeQuery();
 while(rSet.next()) {
 System.out.println("");
 System.out.println("Next Record:");
 System.out.println(rSet.getInt("ID"));
 System.out.println(rSet.getString("firstname"));
 System.out.println(rSet.getString("secondname"));
 System.out.println(rSet.getString("proffesion"));
 }
 }catch (Exception e) {
 e.printStackTrace();
 }finally {
 DBUtil.closeResSet(rSet);
 DBUtil.closePrepStatement(pStatement);
 DBUtil.closeConnection(connection);
 }
}
}

• Tolani: Could you update your question title to reflect what your code is about?
• BenKoshy: why not use an ORM i'm sure they'll be something in java - it saves you do all the plumbing yourself: reading/writing/connecting to db.

Answer 1

• It is usually considered bad practice to use * imports. Be specific about your dependencies.
• Don't use root DB access for your application. That should truly be reserved for a root DB user. Create a MySQL user for the application with appropriate privilege levels.
• Don't hard code your DB credentials into your code. This should ideally be derived from application or environmental configuration.
• Should you really be swallowing up your exceptions like this?
• Your validation against NPE's seems odd here. Rather than a pattern like

this:

try {
 if(con != null) {
 con.close();
 System.out.println("Database Connection Closed!");
 }
}catch(Exception e) {
 e.printStackTrace();
}

perhaps you should be doing something like:

if(con == null) {
 throw new NullPointerException('...');
}
// rest of code in method. Now, without need to be nested in conditional

But you should definitely let the caller know if they passed you a null pointer vs. swallowing it.

• I find it very odd to be opening prepared statements and result sets in your Source class but closing them using the DBUtil class. This seems very asymmetric to me and a leaking of responsibility outside the class.
• Don't get yourself into the bad habit of using SELECT * queries. They can be wasteful of bandwidth (by pulling unnecessary fields from DB into application), they can obfuscate the data structure that is being worked in within the code (i.e. you have to go look at the table in the database to understand what fields you are working with), and it can make your code fragile to table schema changes
• getInput() is a bad method name. This method is actually both reading in user input and inserting data in the database, something that is not implied from the method name, and activities that should probably be split into individual methods. The same could be said about displayData(). This method is not just displaying data, but also retrieving it from the database.
• Consider passing the database connection and the scanner to the Source class as dependencies rather than having the class have to hold the knowledge on how to set them up. Of course, this might require a larger thought shift towards dependency injection throughout other areas of code.
• I know this is a bit of a trivial example, but why would the constructor for the class trigger both the input and ultimate display of content?
• I don't see the need to have class properties for storing the prepared statement and result set objects.
• You don't use dbConnection class property at all. There is probably no need for even storing the connection if you aren't going to reference if across different methods - something not happening the way this class is currently designed.
• I see no need to actually connect twice to the database here, especially considering this class is really designed for the purpose of doing one single operation that is really more procedural in nature.
• When building classes in the future, begin to think about how to separate class functionality from end user messaging or view output. Again, I know this is a simple example, but the more you do this in the future, the more you are better able to make your classes re-usable..

Answer 2

In addition to what @MikeBrand wrote:

Comments

write (non JavaDoc) comments only to explain why your code is like it is. You comments only repead what the code expresses itself which makes them useless at best. Usually this kind of comments tent to stay the same while the code they comment changes and that turns them from something useless into lies.

Naming

Always be careful with your identifier names. Mislieding names will bite you later. eg.:

 DBUtil dbConnection = new DBUtil();

obviously a DBUtil is no connection.

Constructor content

don't do any thing else than populating (final) member variables an (very) basic validation.

Your constructor calls the complete logic of the class.

This means you cannot extend the class because it will do its work before the content of the extending classes constructor is run. This means if your logic calls a method overridden in the subclasses its properties are not initialized yet and you will get NullPointerEcxeptions.

Static methods

Avoid static method.

Static method access makes your code inflexible as well as the use of the new operator does.

You're currently focused on MySql, but what if you want to change to SQL-server or postgreSQL? You would need a separate version of your class Source for each of these.

If you would use inversion of control and dependency injection you would only need new versions of the DBUtil class and a resource file providing the appropriate select statements in the current databases flavor.

Magic numbers

In your code you have literal numbers like this:

 pStatement.setString(1, fName);
 pStatement.setString(2, sName);
 pStatement.setString(3, userProff);

. You should replace them with constants.

Alternatively use a Java enum which could have its own file:

public enum EmployeeInsertAttribute{
 FIRST_NAME(1){
 @Override
 void bind(PreparedStatement pStatement, Object value){
 pStatement.setString(bindVariableIndex, (String)value);
 } 
 }, LAST_NAME(2){
 @Override
 void bind(PreparedStatement pStatement, Object value){
 pStatement.setString(bindVariableIndex, (String)value);
 } 
 }, USER_PROFF(3){
 @Override
 void bind(PreparedStatement pStatement, Object value){
 pStatement.setString(bindVariableIndex, (String)value);
 } 
 }
 }, SALERY(4){
 @Override
 void bind(PreparedStatement pStatement, Object value){
 pStatement.setDouble(bindVariableIndex, (Double)value);
 } 
 }; 
 protected final int bindVariableIndex;
 EmployeeInsertAttribute(int bindVariableIndex){
 this.bindVariableIndex = bindVariableIndex;
 }
 abstract void bind(PreparedStatement pStatement, Object value); 
}

and your code changes to:

 EmployeeInsertAttribute.FIRST_NAME.bind(pStatement, fName);
 EmployeeInsertAttribute.LAST_NAME.bind(pStatement, sName);
 EmployeeInsertAttribute.USER_PROFF.bind(pStatement, userProff);

This looks like a lot of code but think of this:

You have a GUI that only knows the EmployeeInsertAttribute enum and its constants and does not know of the method (since they are package private).

In that GUI you create a Map with the User inputs:

 Map<EmployeeInsertAttribute,Object> userAttributes = new HashMap<>();
 userAttributes.put(EmployeeInsertAttribute.FIRST_NAME,firstNameTextField.getText());
 userAttributes.put(EmployeeInsertAttribute.LAST_NAME,lastNameTextField.getText());
 userAttributes.put(EmployeeInsertAttribute.SALERY,new Double(saleryTextField.getText()));

Then in your current code you can simply iterate over the map:

for(Entry< EmployeeInsertAttribute,Object> attribute: userAttributes)
 attribute.getKey().bind(attribute.getValue());

I know that this approach has some drawbacks aside from being lots of code for the task but it demonstrates the power of java enums...

Separation of Concerns / Single Responsibility Pattern

Your class Source does too much. It deals with the database connection and with the user IO.

You should separate that into different classes.