10
\$\begingroup\$

UPDATE: Added concise problem summary and sample output log.

I tried coding a sample Java program to solve a problem statement:

Concise Problem summary:

Design and Implement an Engine that processes the SMS messages which are sent from a users device (mobile). The messages will come to engine as plain text data.

The message contains instructions to

  1. Process a banking transaction
  2. Modify a users profile

Based on messages mentioned above, the messages can be of two types:

  1. Transaction request that makes changes to the account
  2. Request to modify user profile

The operation corresponding to each type of message can have a pre and post step and the actual operation step.

Important

  1. Assume each user has a fixed. pre-defined and unique

Mobile Number Authentication PIN Transaction PIN Email Id

Populate this information in an in memory data structure / file system which can be used for authentication. Authentication of PIN will involve just validating the mobiie number against PIN numbers and making sure they are same as expected values.

Sample output log

Enter the no of records:
1
Enter the record no:1
anurag|[email protected]|8105720566|7|2708|2708
Record written to the transaction file: anurag|[email protected]|8105720566|7|2708|2708
Enter the no of transactions:
1
Enter the transaction no:1
mo|7|7|200|2708|2708|[email protected]
User found in the transaction file: anurag|[email protected]|8105720566|7|2708|2708
8105720566:PIN authenticated
User found in the transaction file: anurag|[email protected]|8105720566|7|2708|2708
Record written to the transaction file: anurag|[email protected]|8105720566|7|2708|2708
8105720566:Update completed for mo|7|7|200|2708|2708|[email protected]

User:

package com.banking.businessobject;

import java.math.BigInteger;
import java.util.Objects;

public class User {
    private String userName;
    private String email;
    private BigInteger mobileNumber;
    private Account account;

    public User(String userName, String email, BigInteger mobileNumber,
            Account account) {
        this.userName = userName.toLowerCase();
        this.email = email.toLowerCase();
        this.mobileNumber = mobileNumber;
        this.account = account;
    }

    public String getUserName() {
        return userName;
    }

    public void setUserName(String userName) {
        this.userName = userName.toLowerCase();
    }

    public String getEmail() {
        return email;
    }

    public void setEmail(String email) {
        this.email = email.toLowerCase();
    }

    public BigInteger getMobileNumber() {
        return mobileNumber;
    }

    public void setMobileNumber(BigInteger mobileNumber) {
        this.mobileNumber = mobileNumber;
    }

    public Account getAccount() {
        return account;
    }

    public void setAccount(Account account) {
        this.account = account;
    }

    @Override
    public String toString() {
        return userName + "|" + email + "|" + mobileNumber + "|" + account;
    }

    @Override
    public boolean equals(Object obj) {
        User user = (User) obj;
        return (this.userName.equals(user.getUserName())
                && this.email.equals(user.getEmail())
                && this.mobileNumber.intValue() == user.getMobileNumber()
                        .intValue() && this.account.equals(user.getAccount()));

    }

    @Override
    public int hashCode() {
        // TODO Auto-generated method stub
        return Objects.hash(this.userName, this.email, this.mobileNumber,
                this.account);
    }
}

Account:

package com.banking.businessobject;

import java.util.Objects;

public class Account {
    private Integer accountNumber;
    private Integer authPin;
    private Integer transactionPin;

    public Account(Integer accountNumber, Integer authPin,
            Integer transactionPin) {
        this.accountNumber = accountNumber;
        this.authPin = authPin;
        this.transactionPin = transactionPin;
    }

    public Integer getAccountNumber() {
        return accountNumber;
    }

    public void setAccountNumber(Integer accountNumber) {
        this.accountNumber = accountNumber;
    }

    public Integer getAuthPin() {
        return authPin;
    }

    public void setAuthPin(Integer authPin) {
        this.authPin = authPin;
    }

    public Integer getTransactionPin() {
        return transactionPin;
    }

    public void setTransactionPin(Integer transactionPin) {
        this.transactionPin = transactionPin;
    }

    @Override
    public String toString() {
        return accountNumber + "|" + authPin + "|" + transactionPin;
    }

    @Override
    public boolean equals(Object obj) {
        Account account = (Account) obj;
        return (this.accountNumber.intValue() == account.getAccountNumber()
                .intValue()
                && this.authPin.intValue() == account.getAuthPin().intValue() && this.transactionPin
                    .intValue() == account.getTransactionPin().intValue());
    }

    @Override
    public int hashCode() {
        // TODO Auto-generated method stub
        return Objects.hash(this.accountNumber, this.authPin,
                this.transactionPin);
    }
}

Transaction:

package com.banking.businessobject;

public class Transaction {
    // format of transaction message will be as below
    // tx|101|102|20000|pass11|pass22|[email protected]
    // mo| | | |pass11|pass22|[email protected]
    private String transactionType;
    private Integer fromAccountNumber;
    private Integer toAccountNumber;
    private Integer transactionAmount;
    private Integer authPin;
    private Integer transactionPin;
    private String email;

    public Transaction(String[] messageArray) {
        this.transactionType = messageArray[0].toLowerCase();
        this.fromAccountNumber = Integer.valueOf(messageArray[1]);
        this.toAccountNumber = Integer.valueOf(messageArray[2]);
        this.transactionAmount = Integer.valueOf(messageArray[3]);
        this.authPin = Integer.valueOf(messageArray[4]);
        this.transactionPin = Integer.valueOf(messageArray[5]);
        this.email = messageArray[6].toLowerCase();
    }

    public String getTransactionType() {
        return transactionType;
    }

    public void setTransactionType(String transactionType) {
        this.transactionType = transactionType.toLowerCase();
    }

    public Integer getFromAccountNumber() {
        return fromAccountNumber;
    }

    public void setFromAccountNumber(Integer fromAccountNumber) {
        this.fromAccountNumber = fromAccountNumber;
    }

    public Integer getToAccountNumber() {
        return toAccountNumber;
    }

    public void setToAccountNumber(Integer toAccountNumber) {
        this.toAccountNumber = toAccountNumber;
    }

    public Integer getTransactionAmount() {
        return transactionAmount;
    }

    public void setTransactionAmount(Integer transactionAmount) {
        this.transactionAmount = transactionAmount;
    }

    public Integer getAuthPin() {
        return authPin;
    }

    public void setAuthPin(Integer authPin) {
        this.authPin = authPin;
    }

    public Integer getTransactionPin() {
        return transactionPin;
    }

    public void setTransactionPin(Integer transactionPin) {
        this.transactionPin = transactionPin;
    }

    public String getEmail() {
        return email;
    }

    public void setEmail(String email) {
        this.email = email.toLowerCase();
    }

    @Override
    public String toString() {
        return transactionType + "|" + fromAccountNumber + "|" + toAccountNumber + "|" + transactionAmount + "|"
                + authPin + "|" + transactionPin + "|" + email;
    }
}

Implementation/helper classes for FileIO, Sms mocking and Transaction (I have also extracted each of these into an interface):

FileIO:

package com.banking.implementation;

import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.math.BigInteger;

import com.banking.businessobject.Account;
import com.banking.businessobject.User;
import com.banking.interfaces.IFileIO;

public class FileIO implements IFileIO {
    // Test path for Windows machine
//  private final static String TRANSACTION_FILE = "c:\\bank_transaction.txt";

    // Test path for MacBook
     private final static String TRANSACTION_FILE =
     "/Users/anurag/bank_transaction.txt";


    @Override
    public boolean intitalizeTransactionFile() {
        // Initialize the file
        try {
            File file = new File(TRANSACTION_FILE);

            if (file.createNewFile()) {
                System.out.println("Transaction File created");
            } else {
                System.out.println("Transaction File already exists");
            }
        } catch (IOException e) {
            e.printStackTrace();
            // check if this actually returns false
            return false;
        }
        return true;
    }


    @Override
    public User readTransactionFile(User user) {

        BufferedReader br = null;
        String currentLine = "";

        try {
            br = new BufferedReader(new FileReader(TRANSACTION_FILE));
            while ((currentLine = br.readLine()) != null) {
                if (currentLine.contains(user.getEmail().toLowerCase())) {
                    break;
                }
            }

        } catch (IOException e) {
            e.printStackTrace();
        }

        try {
            br.close();
        } catch (IOException e) {
            e.printStackTrace();
        }

        String[] objectValue = currentLine.trim().split("\\|");
        Account account = new Account(Integer.valueOf(objectValue[3]),
                Integer.valueOf(objectValue[4]),
                Integer.valueOf(objectValue[5]));
        User newUser = new User(objectValue[0], objectValue[1], new BigInteger(
                objectValue[2]), account);
        System.out.println("User found in the transaction file: "
                + newUser.toString());
        return newUser;

        // to add exception code here if the record is not found
    }


    @Override
    public boolean writeTransactionFile(User user) {
        // **important**
        // Eclipse needs to be *Run as Admin* to have the write access to write
        // to the file
        // Problem with Windows 7+ as of now. Still need to check on Mac.

        BufferedReader br = null;
        BufferedWriter bw = null;
        String currentLine = "";

        try {
            br = new BufferedReader(new FileReader(TRANSACTION_FILE));
            bw = new BufferedWriter(new FileWriter(TRANSACTION_FILE + ".tmp",
                    true));

            while ((currentLine = br.readLine()) != null) {
                if (currentLine.contains(user.getMobileNumber().toString()
                        .toLowerCase())) {
                    continue;
                }
                bw.write(currentLine);
                bw.newLine();
            }
            bw.write(user.toString());

        } catch (Exception e) {
            e.printStackTrace();
        }

        try {
            // check if flush is important
            bw.flush();
            br.close();
            bw.close();
        } catch (IOException e) {
            e.printStackTrace();
        }
        File oldFile = new File(TRANSACTION_FILE);
        File newFile = new File(TRANSACTION_FILE + ".tmp");
        if (oldFile.delete()) {
            newFile.renameTo(oldFile);
        }

        // again check for error handling here
        System.out.println("Record written to the transaction file: "
                + user.toString());
        return true;
    }
}

Sms:

package com.banking.implementation;

import com.banking.businessobject.Transaction;
import com.banking.businessobject.User;
import com.banking.interfaces.ISmsGateway;
import com.banking.interfaces.ITransaction;

public class SmsGatewayImpl implements ISmsGateway {

    private String authPinNotification = "";
    private String transPinNotification = "";
    private String transactionNotification = "";
    private String profileUpdateNotification = "";

    public String getAuthPinNotification() {
        return authPinNotification;
    }

    public String getTransPinNotification() {
        return transPinNotification;
    }

    public String getTransactionNotification() {
        return transactionNotification;
    }

    public String getProfileUpdateNotification() {
        return profileUpdateNotification;
    }

    @Override
    public void receiveNextMessage(String message, User user) {
        // format of transaction message will be as below
        // tx|101|102|20000|pass11|pass22|[email protected]
        // mo| | | |pass11|pass22|[email protected]
        String[] messageArray = message.trim().split("\\|");
        Transaction trnObj = new Transaction(messageArray);
        ITransaction trnImpl = new TransactionImpl();

        boolean authenticated = trnImpl.accountAuthentication(user, trnObj);

        switch (trnObj.getTransactionType().toLowerCase()) {
        case "tx":
            authPinNotification = notifyPinAuthenticationResult(user, authenticated);
            if (!authPinNotification.isEmpty() && authPinNotification != null) {
                System.out.println(authPinNotification);
            }
            if (trnObj.getTransactionAmount() > 1000) {
                transPinNotification = notifyTransPinAuthenticationResult(user, authenticated);

                if (!transPinNotification.isEmpty() && transPinNotification != null) {
                    System.out.println(transPinNotification);
                }
            }
            transactionNotification = notifyTransactionResult(user, trnObj, authenticated);
            if (!transactionNotification.isEmpty() && transactionNotification != null) {
                System.out.println(transactionNotification);
            }
            break;

        case "mo":
            authPinNotification = notifyPinAuthenticationResult(user, authenticated);
            if (!authPinNotification.isEmpty() && authPinNotification != null) {
                System.out.println(authPinNotification);
            }

            trnImpl.profileUpdate(user, trnObj);

            profileUpdateNotification = notifyProfileUpdateResult(user, trnObj, authenticated);

            if (!profileUpdateNotification.isEmpty() && profileUpdateNotification != null) {
                System.out.println(profileUpdateNotification);
            }
            break;

        default:
            // add error handling code here
            System.out.println("Incorrect Transaction Query format");
            break;
        }
    }

    @Override
    public String notifyPinAuthenticationResult(User user, boolean authentication) {
        StringBuilder string = new StringBuilder();
        if (authentication) {
            string.append(user.getMobileNumber().toString());
            string.append(":PIN authenticated");
            return string.toString();
        }
        string.append("Error:");
        string.append(user.getMobileNumber());
        string.append(":PIN authentication failed");
        return string.toString();
    }

    @Override
    public String notifyTransPinAuthenticationResult(User user, boolean authentication) {
        StringBuilder string = new StringBuilder();
        if (authentication) {
            string.append(user.getMobileNumber().toString());
            string.append(":TPIN authenticated");
            return string.toString();
        }
        string.append("Error:");
        string.append(user.getMobileNumber());
        string.append(":TPIN authentication failed");
        return string.toString();
    }

    @Override
    public String notifyTransactionResult(User user, Transaction trnObj, boolean transaction) {
        if (transaction) {
            return (user.getMobileNumber() + ":Txn completed for " + trnObj.toString());
        }
        return ("Error:" + user.getMobileNumber() + ":Txn failed for " + trnObj.toString());
    }

    @Override
    public String notifyProfileUpdateResult(User user, Transaction trnObj, boolean authenticated) {
        if (authenticated) {
            return (user.getMobileNumber() + ":Update completed for " + trnObj.toString());
        }
        return ("Error:" + user.getMobileNumber() + ":Update failed for " + trnObj.toString());
    }

}

Transaction:

package com.banking.implementation;

import com.banking.businessobject.Transaction;
import com.banking.businessobject.User;
import com.banking.interfaces.IFileIO;
import com.banking.interfaces.ITransaction;

public class TransactionImpl implements ITransaction {
    @Override
    public boolean accountAuthentication(User user, Transaction trnObj) {
        boolean authentication = false;
        IFileIO fi = new FileIO();
        User userRecord = fi.readTransactionFile(user);
        if (userRecord.getMobileNumber().intValue() == user.getMobileNumber()
                .intValue()
                && userRecord.getAccount().getAuthPin().intValue() == user
                        .getAccount().getAuthPin().intValue()) {
            if (trnObj.getTransactionType().toLowerCase().equals("tx")
                    && trnObj.getTransactionAmount().intValue() > 1000) {
                if (userRecord.getAccount().getTransactionPin().intValue() != user
                        .getAccount().getTransactionPin().intValue()) {
                    return authentication;
                }
            }

            authentication = true;
        }
        return authentication;
    }

    @Override
    public boolean profileUpdate(User user, Transaction trnObj) {

        IFileIO fi = new FileIO();

        User userRecord = fi.readTransactionFile(user);
        userRecord.setEmail(trnObj.getEmail());

        return fi.writeTransactionFile(userRecord);
    }
}

I have also created unit tests for the helper classes, which I think is not important to attach here.

I am just looking for honest feedback on the design, coding, or anything else on the code.

\$\endgroup\$
3
  • \$\begingroup\$ Could you include a small summary of the problem statement? We do not like relying much on external links \$\endgroup\$
    – Caridorc
    Dec 20, 2015 at 10:53
  • 1
    \$\begingroup\$ @Caridorc Thanks for pointing out! Added summary of the problem statement and sample output too. \$\endgroup\$
    – Anurag
    Dec 20, 2015 at 11:21
  • \$\begingroup\$ Thanks for caring about improving the quality of your post, I hope we had more users like you. \$\endgroup\$
    – Caridorc
    Dec 20, 2015 at 11:34

2 Answers 2

4
\$\begingroup\$

TransactionImpl#accountAuthentication

I'll start with accountAuthentication method from TransactionImpl:

@Override
public boolean accountAuthentication(User user, Transaction trnObj) {
    boolean authentication = false;
    IFileIO fi = new FileIO();
    User userRecord = fi.readTransactionFile(user);
    if (userRecord.getMobileNumber().intValue() == user.getMobileNumber()
            .intValue()
            && userRecord.getAccount().getAuthPin().intValue() == user
            .getAccount().getAuthPin().intValue()) {
        if (trnObj.getTransactionType().toLowerCase().equals("tx")
                && trnObj.getTransactionAmount().intValue() > 1000) {
            if (userRecord.getAccount().getTransactionPin().intValue() != user
                    .getAccount().getTransactionPin().intValue()) {
                return authentication;
            }
        }

        authentication = true;
    }
    return authentication;
}

You can extract the big if in a separate method:

@Override
public boolean accountAuthentication(User user, Transaction trnObj) {
    boolean authentication = false;
    IFileIO fi = new FileIO();
    User userRecord = fi.readTransactionFile(user);
    if (isBadAuthentication(user, trnObj, userRecord)) {
        return authentication;
    }
    authentication = true;

    return authentication;
}

Then we can get rid of authentication temp variable:

public boolean accountAuthentication2(User user, Transaction trnObj) {
    IFileIO fi = new FileIO();
    User userRecord = fi.readTransactionFile(user);
    return verifyAuthentication(user, trnObj, userRecord);
}

Now for the verifyAuthentication method, I extracted each condition in a separate method and inverted the negative conditions. If we need to check for a negative, we can use !.

private boolean verifyAuthentication(User user, Transaction trnObj, User userRecord) {
    return verifyMobilePhone(user, userRecord) &&
           verifyAuthPin(user, userRecord) &&
           !isTxTransaction(trnObj) &&
           !hasBigAmount(trnObj) &&
           verifyTransactionPin(user, userRecord);
}

private boolean verifyTransactionPin(User user, User userRecord) {
    return userRecord.getAccount().getTransactionPin().intValue() ==
           user.getAccount().getTransactionPin().intValue();
 }

private boolean hasBigAmount(Transaction trnObj) {
    return trnObj.getTransactionAmount().intValue() > 1000;
}

private boolean isTxTransaction(Transaction trnObj) {
    return trnObj.getTransactionType().toLowerCase().equals("tx");
}

private boolean verifyAuthPin(User user, User userRecord) {
    return userRecord.getAccount().getAuthPin().intValue() ==
           user.getAccount().getAuthPin().intValue();
}

private boolean verifyMobilePhone(User user, User userRecord) {
    return userRecord.getMobileNumber().intValue() ==
           user.getMobileNumber().intValue();
}

We have many small methods now, but we can easily move them to their related class. For example, we move isTxTransaction and hasBigAmount right in the Transaction class:

Transaction.java :

public boolean isTx() {
    return getTransactionType().toLowerCase().equals("tx");
}

public boolean hasBigAmount() {
    return getTransactionAmount().intValue() > 1000;
}

TransactionImpl.java :

private boolean verifyAuthentication(User user, Transaction trnObj, User userRecord) {
    return verifyMobilePhone(user, userRecord) &&
           verifyAuthPin(user, userRecord) &&
           !trnObj.isTx() &&
           !trnObj.hasBigAmount() &&
           verifyTransactionPin(user, userRecord);
}

Integers are automatically unboxed, there is no need to call intValue in hasBigAmount:

public boolean hasBigAmount() {
    return getTransactionAmount() > 1000;
}

SmsGatewayImpl#receiveNextMessage

receiveNextMessage has this big switch:

switch (trnObj.getTransactionType().toLowerCase()) {
    case "tx":
        authPinNotification = notifyPinAuthenticationResult(user, authenticated);
        if (!authPinNotification.isEmpty() && authPinNotification != null) {
            System.out.println(authPinNotification);
        }
        if (trnObj.getTransactionAmount() > 1000) {
            transPinNotification = notifyTransPinAuthenticationResult(user, authenticated);

            if (!transPinNotification.isEmpty() && transPinNotification != null) {
                System.out.println(transPinNotification);
            }
        }
        transactionNotification = notifyTransactionResult(user, trnObj, authenticated);
        if (!transactionNotification.isEmpty() && transactionNotification != null) {
            System.out.println(transactionNotification);
        }
        break;

    case "mo":
        authPinNotification = notifyPinAuthenticationResult(user, authenticated);
        if (!authPinNotification.isEmpty() && authPinNotification != null) {
            System.out.println(authPinNotification);
        }

        trnImpl.profileUpdate(user, trnObj);

        profileUpdateNotification = notifyProfileUpdateResult(user, trnObj, authenticated);

        if (!profileUpdateNotification.isEmpty() && profileUpdateNotification != null) {
            System.out.println(profileUpdateNotification);
        }
        break;

    default:
        // add error handling code here
        System.out.println("Incorrect Transaction Query format");
        break;
}

authPinNotification is printed in both cases, it can be extracted in a separate method. You can also extract each case in a method. I also used the hasBigAmount method extracted earlier.

printAuthPinNotification(user, authenticated);
switch (trnObj.getTransactionType().toLowerCase()) {
    case "tx":
        processTxTransaction(user, trnObj, authenticated);
        break;

    case "mo":
        processMoTransaction(user, trnObj, trnImpl, authenticated);
        break;

    default:
        // add error handling code here
        System.out.println("Incorrect Transaction Query format");
        break;
    }
}

private void printAuthPinNotification(User user, boolean authenticated) {
    authPinNotification = notifyPinAuthenticationResult(user, authenticated);
    if (authPinNotification != null && !authPinNotification.isEmpty()) {
        System.out.println(authPinNotification);
    }
}

private void processMoTransaction(User user, Transaction trnObj, ITransaction trnImpl, boolean authenticated) {
    trnImpl.profileUpdate(user, trnObj);

    profileUpdateNotification = notifyProfileUpdateResult(user, trnObj, authenticated);

    if (profileUpdateNotification != null && !profileUpdateNotification.isEmpty()) {
        System.out.println(profileUpdateNotification);
    }
}


private void processTxTransaction(User user, Transaction trnObj, boolean authenticated) {
    if (trnObj.hasBigAmount()) {
        transPinNotification = notifyTransPinAuthenticationResult(user, authenticated);

        if (transPinNotification != null && !transPinNotification.isEmpty()) {
            System.out.println(transPinNotification);
        }
    }
    transactionNotification = notifyTransactionResult(user, trnObj, authenticated);
    if (transactionNotification != null && !transactionNotification.isEmpty()) {
        System.out.println(transactionNotification);
    }
}

Transaction type

Consider having an enum for transaction type.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot! Subtle changes but makes a lot of sense in design. \$\endgroup\$
    – Anurag
    Dec 20, 2015 at 10:18
3
\$\begingroup\$
@Override
public int hashCode() {
    // TODO Auto-generated method stub <- this line here
    return Objects.hash(this.userName, this.email, this.mobileNumber,
            this.account);
}

Umm... I think your hashCode() method is done...

This:

    BufferedReader br = null;
    BufferedWriter bw = null;
    String currentLine = "";

    try {
        br = new BufferedReader(new FileReader(TRANSACTION_FILE));
        bw = new BufferedWriter(new FileWriter(TRANSACTION_FILE + ".tmp",
                true));

        while ((currentLine = br.readLine()) != null) {
            if (currentLine.contains(user.getMobileNumber().toString()
                    .toLowerCase())) {
                continue;
            }
            bw.write(currentLine);
            bw.newLine();
        }
        bw.write(user.toString());

    } catch (Exception e) {
        e.printStackTrace();
    }

    try {
        // check if flush is important
        bw.flush();
        br.close();
        bw.close();
    } catch (IOException e) {
        e.printStackTrace();
    }

First thing's first. Never catch Exception. Always catch the specific exception that might be thrown. In this case, IOException (I believe).

Next, it can be improved by using Java 7's try-with-resources:

    try (BufferedReader br = new BufferedReader(new FileReader(TRANSACTION_FILE),
            BufferedWriter bw = new BufferedWriter(
                    new FileWriter(TRANSACTION_FILE + ".tmp", true))) {
        String currentLine = "";
        while ((currentLine = br.readLine()) != null) {
            if (currentLine.contains(user.getMobileNumber().toString()
                    .toLowerCase())) {
                continue;
            }
            bw.write(currentLine);
            bw.newLine();
        }
        bw.write(user.toString());
        bw.flush();
    } catch (Exception e) {
        e.printStackTrace();
    }

Disclaimer: not tested.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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