Asking a user to take money from a checking account and move it to a savings account - follow-up

Question

I made a few changes to this:

Asking a user to take money from a checking account and move it to a savings account

The changes I made were:

• improved the names of methods and variables to better describe what they do
• passed the db credentials to a properties file
• set a different db user (from 'root' to 'test_user')
• added/removed several functions/variables, among other minor changes (copy/paste issues, spacing, etc)

I'm still hesitant about the way the db variables are handled/called in my MainApplication class. Is the initializeDatabaseVariables method something you find acceptable or would you rather go ahead with creating a different class to handle the database connection (initialize, connect, close)?

Below is the modified source code. If there's still anything that sticks out which you suggest should be changed please don't hesitate to tell me.

config.properties

#Mon Jun 02 21:37:18 CEST 2014
port=3306
hostname=localhost
password=
database=jdbc_example
username=test_user

MainApplication class

package com.jdbcbank;

import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.Properties;
import java.util.Scanner;

public class MainApplication {

    // Database access
    static String DB_NAME;
    static String HOSTNAME;
    static String DB_PORT;
    static String DB_URL;

    // Database credentials
    static String DB_USR;
    static String DB_PASS;

    // Connection variables
    static Connection conn = null;
    static Statement statement = null;
    static ResultSet result = null;
    static PreparedStatement pStatement = null;

    public static void main(String[] args) throws SQLException {

        initializeDatabaseVariables();

        int testUserId = 1;
        int accountBalances[];

        try {

            // Connect to database
            MainApplication.connectToDb();

            // Get current balance
            BankAccount bankAccount = new BankAccount();

            accountBalances = bankAccount.getAccountBalances(testUserId);
            System.out.println("---------------");
            System.out.println("Current balance ");
            System.out.println("---------------");
            System.out.println("Checking Account: " + accountBalances[0]+ " USD");              
            System.out.println("Savings Account: " + accountBalances[1]+ " USD\n");

            checkIfValueIsNegative(accountBalances[0]); 

            System.out
                    .print("Enter the amount (in USD) you wish to move to your savings account: ");

            // Run transaction
            Scanner transferAmountScanner = new Scanner(System.in);     
            int transferAmount = transferAmountScanner.nextInt();
            transferAmountScanner.close();  

            checkIfValueIsOutOfRange(testUserId, transferAmount);
            checkIfValueIsNegative(transferAmount);     
            bankAccount.moveAmountFromCheckingToSavings(transferAmount);

            // Get new balance
            accountBalances = bankAccount.getAccountBalances(testUserId);
            System.out.println();
            System.out.println("-----------");
            System.out.println("New balance ");
            System.out.println("-----------");
            System.out.println("Checking Account: " + accountBalances[0]+ " USD");
            System.out.println("Savings Account: " + accountBalances[1]+ " USD\n");
            System.out.println("Thank you.");

        } catch (Exception e) {
            System.out.println("Error: " + e.getMessage());
        } finally {
            // Close all connections
            close(conn, statement, pStatement, result);
        }

    }

    public static void initializeDatabaseVariables() {

        Properties prop = new Properties();
        InputStream input = null;

        try {

            input = new FileInputStream("config.properties");

            // Load properties file
            prop.load(input);

            // Initialize database variables
            DB_NAME = prop.getProperty("database");
            HOSTNAME = prop.getProperty("hostname");
            DB_PORT = prop.getProperty("port");
            DB_USR = prop.getProperty("username");
            DB_PASS = prop.getProperty("password");
            DB_URL = "jdbc:mysql://" + HOSTNAME + ":" + DB_PORT + "/" + DB_NAME;

        } catch (IOException e) {
            System.out.println("Error: " + e.getMessage());
        } finally {
            if (input != null) {
                try {
                    input.close();
                } catch (IOException e) {
                    System.out.println("Error: " + e.getMessage());
                }
            }
        }
    }

    public static void checkIfValueIsNegative(int value) {

        if (value < 0) {
            throw new IllegalArgumentException(
                    "Oops! Negative values not allowed.");
        }

    }

    public static void checkIfValueIsOutOfRange(int testUserId,
            int transferAmount) throws SQLException {

        int checkingBalance = new BankAccount().getAccountBalances(testUserId)[0];

        if (transferAmount > checkingBalance) {
            throw new IllegalArgumentException("Oops! Insuficient funds to transfer.");
        }
    }

    public static void connectToDb() {

        try {
            // Load driver
            Class.forName("com.mysql.jdbc.Driver");

            // Set connection
            conn = DriverManager.getConnection(DB_URL, DB_USR, DB_PASS);
            conn.setAutoCommit(false);
            statement = conn.createStatement();

        } catch (ClassNotFoundException e) {
            System.out.println("Error: " + e.getMessage());
        } catch (SQLException e) {
            System.out.println("Error: " + e.getMessage());
        }
    }

    public static void close(Connection conn, Statement statement,
            PreparedStatement pStatement, ResultSet result) {
        if (conn != null)
            try {
                conn.close();
            } catch (SQLException e) {
                System.out.println("Error: " + e.getMessage());
            }
        if (statement != null)
            try {
                statement.close();
            } catch (SQLException e) {
                System.out.println("Error: " + e.getMessage());
            }
        if (pStatement != null)
            try {
                pStatement.close();
            } catch (SQLException e) {
                System.out.println("Error: " + e.getMessage());
            }
        if (result != null)
            try {
                result.close();
            } catch (SQLException e) {
                System.out.println("Error: " + e.getMessage());
            }
    }

}

BankAccount class

package com.jdbcbank;

import java.sql.*;

public class BankAccount {

    public void moveAmountFromCheckingToSavings(int transferAmount) throws SQLException {

        try {
            // Withdraw from checking and deposit into saving
            withdrawFromChecking(MainApplication.statement, transferAmount, 1);
            depositIntoSaving(MainApplication.statement, transferAmount, 1);

            // Execute batch and commit if no issues are found
            MainApplication.statement.executeBatch();
            MainApplication.conn.commit();

            // Print message
            System.out.println("Successful transaction!");

        } catch (Exception e) {
            System.out.println("Error: " + e.getMessage());
            MainApplication.conn.rollback();
        }
    }

    public int[] getAccountBalances(int id) throws SQLException {

        // Array to be returned
        int balance[] = new int[2];

        // Get results
        String query = "SELECT * from bank_account WHERE id = " + id;
        MainApplication.result = MainApplication.statement.executeQuery(query);

        while (MainApplication.result.next()) {
            // Retrieve by column name
            balance[0] = MainApplication.result.getInt("checking_balance");
            balance[1] = MainApplication.result.getInt("saving_balance");
        }
        return balance;

    }

    public static void withdrawFromChecking(Statement statement, int amount,
            int id) throws SQLException {

        statement
                .addBatch("UPDATE bank_account SET checking_balance = checking_balance - "
                        + amount + " WHERE id = " + id);

    }

    public static void depositIntoSaving(Statement statement, int amount, int id)
            throws SQLException {

        statement
                .addBatch("UPDATE bank_account SET saving_balance = saving_balance + "
                        + amount + " WHERE id = " + id);
    }

}

Answer 1

General suggestions

• Find ways to shorten your methods.
• DRY (don't repeat yourself).

Specific points

• Combining the above points: extract your series of printlns to a new BankAccount.displayBalances() method. This will also allow you to get rid of the accountBalances variable.
• You have too many global variables. Keep the Connection for a while, but all the others can be local, with very small scopes. There's no overhead in creating new Statements or ResultSets, and it's easy for bugs to creep in when you reuse variables like that.
• See if you can reduce your main method to a brief list of imperatives, with parameters passed from one to the next when necessary. Things like "connectToDb", "loadAccount", "transferUserSpecifiedAmount" and "displayBalances" are enough. You can find better places for things like new Scanner, checkIfValueIsOutOfRange and println.
• Don't have business logic in your main method. You might move the logic to a new method boolean validateMoveAmountFromCheckingToSavings(int), or have the method moveAmountFromCheckingToSavings throw an exception if the business logic forbids it.
• In relation to your question about initializeDatabaseVariables: it would probably be best to avoid global variables. Perhaps create a Properties MainApplication.loadProperties() method, and retain the returned properties in your main method. Change connectToDb() to take that Properties as a parameter. You may even want to create String MainApplication.createConnectionString(Properties), which is quite self-documenting.
• Read up on what all the close methods in Connection, Statement, PreparedStatement and ResultSet do. You may find that you don't need to close everything.

Answer 2

I think a bit of redesign of your class hierarchy would pay dividends. The entities you have seem to be:

• A database that contains the account information
• A source account (stored in the database)
• A destination account (also stored in the database)

Based on this you would create one instance of the main application, and two instances of BankAccount (source and destination account).

The main application would create a database connection, then construct two accounts (of same class) for source and destination (or checking/saving whatever). The constructor of BankAccount would take the database connection as an argument. So all the main app would need to do is:

• Start a database transaction
• Transfer from the source account to the destination account. This should be as simple as destinationAccount.deposit(sourceAccount.withdraw(amount)); Insufficient funds should throw an exception - this needs to be caught and a database rollback performed.
• Commit the database transaction

One sign of a good class hierarchy is the total absence of static methods - i.e. every action acts on an object. Your current design has methods in the main application that belong in the BankAccount class. For instance depositIntoSaving(amount) should be savingsAccount.deposit(amount)

Also, I dislike checked exceptions (e.g. SQLException). I tend to catch these at source and turn them into RunTimeException instances, which don't clutter your code with unnecessary throws declarations - but this is personal preference, and I may be starting a religious war.

Good luck!