Email report generation from database

Question

I have written a small batch job which will collect data from db and send mail to user. Can you please do a review of the code with design prinicples in mind and also with best practices for Db and file writer. I am writing to file in this code and sending it in mail.

The user will call the class with report type to be generated along with startdate and enddate.

There could be 3-4 type of report generation types so user can give comma separated list of report types to be generated. I take the report type and generate the class related to it.

I want to write an extensible code or maintable code.In these set of classes CSVwriter and Report Dao do play major part . I want to keep a single procedure from where differnt reports get data , hence report dao method getDataForRepot is used for all different reports similarly CSV writer is used to write data to csv file for all report.

Please do close review of Reprot Dao and CSVwriter class.Is there need of VO or DTO ?

Below is the main class which calls report generator class basically

import java.util.ResourceBundle;


public class ReportsManager {

    public static void main(String[] args) {

        try {

        if (args.length > 0) {
            String[] reportsList = null;
            if (args[0] != null && args[0].trim().length() != 0) {
                reportsList = args[0].split(",");
            }
            String startDate = null;
            if (args[1] != null && args[1].trim().length() != 0) {
                startDate = args[1];
            }
            String endDate = null;
            if (args[2] != null && args[2].trim().length() != 0) {
                startDate = args[2];
            }

            final ResourceBundle BUNDLE = ResourceBundle
            .getBundle(AppConstants.CONFIG_PATH);
            for (int i = 0; i < reportsList.length; i++) {
                final String clazzName = BUNDLE.getString(reportsList[i]);

                BatchJLogger.logMessage(" Report Generation Started for  "
                        + reportsList[i]);
                String fileDetails[] = null;
                ReportsInterface reports = (ReportsInterface) Class
                .forName(clazzName).newInstance();
                fileDetails = reports.execute(startDate, endDate,
                        reportsList[i]);

                String strBody = reports.getMailContent();
                MailSender mailSender = new MailSender(reportsList[i]);
                mailSender.sendMail(strBody, fileDetails[0],
                        fileDetails[1], true);
            }
        } else {
            System.err
            .println(" Please enter the report type to be generated");
        }

    } catch (IOException ex) {
        ex.printStackTrace();
    } catch (SQLException ex) {
        ex.printStackTrace();
    } catch (Exception ex) {
        ex.printStackTrace();
    }
}
}

Below is the interface which all report types should implement

public interface ReportsInterface {

    public String[] execute(String startDate,String endDate,String reportType) throws IOException,SQLException,Exception;

    /**
     * called after execute. 
     */
    public String getMailContent();
}

Below is one of the report class which creates file and then calls Db to get data using Stored procedure then writes content to File and send mail

import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.util.Calendar;
import java.util.Vector;


public class UserDetailsReport implements ReportsInterface {

  private Map<Integer, TreeMap<String,String>> userDetialsData =null;
String dateTimeStamp;

    @Override
    public String[] execute(String startDate, String endDate,String  reportType) throws IOException,SQLException,Exception{
    String[] fileName=null;
    ReportDao reportDao = new ReportDao();
     userDetialsData = reportDao.getDataForRepot(startDate,
            endDate,reportType);
        CsvWriter csvWriter = new CsvWriter();

        fileName=csvWriter.writeDetailsToFile(reportType, callMeReportData,startDate,endDate);
        dateTimeStamp=fileName[2];
    return fileName;

}

    public String getMailContent(){
        StringBuilder body = new StringBuilder();
        body.append("\n");
        body.append("\n");
        body.append("Please find attached report.");
        body.append("\n");
        body.append("\n");
        body.append("\n");
        body.append("\n");
        body.append("Thanks,");
        return body.toString(); 
    }

}

Below is the class with creates and writes data to csv files

    import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Calendar;
import java.util.HashMap;
import java.util.Map;
import java.util.ResourceBundle;
import java.util.Set;
import java.util.TreeMap;



public class CsvWriter {
    // this will hold the list of header for each reprot so that i dont have to write any specific method for a particular report
    private static Map<String,ArrayList<String>> headers = new HashMap<String, ArrayList<String>>(); 

    static{
        ArrayList<String> callMe = new ArrayList<String>();
        callMe.add("Mobile Phone");
        callMe.add("Call Time");
        callMe.add("Submitted On");
        callMe.add("First Name");
        callMe.add("Last Name");
        callMe.add("Email");
        ArrayList<String> leadGen = new ArrayList<String>();
        leadGen.add("First Name");
        leadGen.add("Last Name");
        leadGen.add("Mobile Phone");
        leadGen.add("Product Interested");
        leadGen.add("Submitted On");
        headers.put("USERDETAILS_REPORT", callMe);
        headers.put("SOMEOTHER_REPORT", leadGen);
    }

    private FileWriter fileWriter = null;
    private String folderName = null;
    public enum ReportType{USERDETAILS_REPORT,SOMEOTHER_REPORT};

    public FileWriter getFileWriter() {
        return fileWriter;
    }

    public void setFileWriter(FileWriter fileWriter) {
        this.fileWriter = fileWriter;
    }

    final ResourceBundle BUNDLE = ResourceBundle
            .getBundle(AppConstants.CONFIG_PATH);

    public CsvWriter() {
        folderName = BUNDLE.getString("FOLDER_PATH");
    }

    // this method will be used by all the reprots

    public String[] writeDetailsToFile(String reportType,Map<Integer, TreeMap<String,String>> values,String startDate,String endDate) throws IOException,Exception {
        String filePath=null;
        String fileName =null;
        String[] details=null;
        try{

            //BatchJLogger.logMessage(" Started Execution of method writeDetailsToFile " );
            BatchJLogger.logMessage(" Started Execution of method openXls " );

            details = createFileName(reportType, startDate, endDate);
            fileName=details[0];
            filePath = folderName + File.separator + fileName;
            fileWriter = new FileWriter(filePath);
            File f = new File(filePath);
            BatchJLogger.logMessage(" file created "+f.exists() );

            fileWriter.write("Report Name");
            fileWriter.write(",");
            switch (ReportType.valueOf(reportType)) {
            case USERDETAILS_REPORT:
                fileWriter.write("USER DETILS");
                break;
            case SOMEOTHER_REPORT:
                fileWriter.write("SOME");
                break;
            default:
                break;
            }
            fileWriter.write(",");
            fileWriter.write("Date ");
            fileWriter.write(",");
            fileWriter.write(CsvWriter.getCurrentDate());
            fileWriter.write("\n");
            fileWriter.write("\n");
            ArrayList<String> cloumnNames = headers.get(reportType);
            int fileHeader = 0;
            for (String columnName : cloumnNames) {
                fileWriter.write(columnName);
                if(fileHeader < cloumnNames.size()){
                    fileWriter.write(",");
                }
                fileHeader++;
            }
            fileWriter.write("\n");


            Set<Integer> recordSet = values.keySet();
            for (Integer record : recordSet) {
                TreeMap<String, String> data = values.get(record);
                int columnCount = 0;
                for (String columnName : cloumnNames) {
                    String columnData=data.get(columnName);
                    if((columnName.equalsIgnoreCase("Mobile Phone")||columnName.equalsIgnoreCase("Submitted On")) && columnData!=null ){
                        fileWriter.write("'");
                    }
                    fileWriter.write(BatchJUtil.checknull(columnData));
                    if(columnCount < cloumnNames.size()){
                        fileWriter.write(",");
                    }
                    columnCount++;
                }
                fileWriter.write("\n");
            }
        }
        finally{
            fileWriter.flush();
            fileWriter.close(); 
            return new String[]{filePath,fileName,details[1]};
        }

        //BatchJLogger.logMessage(" end of Execution of method writeDetailsToFile " );
    }



    public String[] createFileName(String reportType,String startDate,String endDate) throws ParseException{
        String[] data =null;
        String fileName=null;
        String toDaysDate=null;
        if(startDate!=null && startDate.length()!=0){
            startDate=BatchJUtil.convertformat(startDate);
            endDate=BatchJUtil.convertformat(endDate);
            toDaysDate=startDate+"_To_"+endDate;
        }else{
            DateFormat dateFormat = new SimpleDateFormat("dd-MMM-yyyy");
            Calendar cal = Calendar.getInstance();
            String date = dateFormat.format(cal.getTime());
            String[] parsedDate = date.split("-");
            int numDay = Integer.parseInt(parsedDate[0]);
            String  month = parsedDate[1];
            int numYear = Integer.parseInt(parsedDate[2]);
            toDaysDate =  BatchJUtil.checkNumber(numDay) + "-"+month+ "-" + BatchJUtil.checkNumber(numYear);    
        }



        switch (ReportType.valueOf(reportType)) {
        case USERDETAILS_REPORT:
            fileName="UserDetails_"+toDaysDate+".csv";
            break;
        case SOMEOTHER_REPORT:
            fileName="SomeOther_"+toDaysDate+".csv";
            break;
        default:
            break;
        }
        data=new String[]{fileName,toDaysDate};
        return data;
    }

    public static String getCurrentDate(){
        DateFormat dateFormat = new SimpleDateFormat("dd-MMM-yyyy");
        Calendar cal = Calendar.getInstance();
        return dateFormat.format(cal.getTime());
    }


}

This is the class which gets connection from Db

import java.sql.CallableStatement;
import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ResourceBundle;
import java.util.Vector;
import javax.sql.DataSource;


public class RConnection {
    private Connection mConnection;
    private Statement mStatement;
    private static Object lockObject = new Object();
    private static int openCount = 0;
    private static int closeCount = 0;
    private static String dataSourceName = "somedb";
    private static DataSource dataSource = null;
    private Vector statementVector= new Vector();
    private Vector resultSetVector= new Vector();


    public CallableStatement createCallableStatement(String sql) throws SQLException
    {
        CallableStatement cstmt = null;
        try
        {
            cstmt = this.getDBConnection().prepareCall(sql);
            statementVector.add(cstmt);
        }
        catch(SQLException e)
        {       
            e.printStackTrace();
            throw new SQLException("Can not create CallableStatement for sql "+e.getMessage());
        }
        return cstmt;
    }


    public ResultSet getResultSet(CallableStatement cstmt, int paramNo) throws SQLException
    {
        ResultSet rs=null;
        try
        {
            rs=(ResultSet) cstmt.getObject(paramNo);
            resultSetVector.add(rs);
        }
        catch(SQLException e)
        {
            e.printStackTrace();
            throw new SQLException("Can not retrieve ResultSet for this CallableStatement "+e.getMessage());
        }
        return rs;
    }


    public static int getOpenConnections()
    {
        return openCount;
    }
    public static int getCloseConnections()
    {
        return closeCount;
    }
    /**
    /**
     * Constructor being made Private, Singleton implementation
     */
    public RConnection()
    {
        this.connect();
    }


    /**
     * This is the function that is used to connect to the
     * database using jdbc
     */
    public void connect()
    {
        try
        {
            String errorString = "Error obtaining database connection.";
            final ResourceBundle BUNDLE = ResourceBundle.getBundle(AppConstants.CONFIG_PATH);

                try {

                    String DB_USERNAME = BUNDLE.getString(AppConstants.DB_USERNAME);
                    String DB_PASSWORD = BUNDLE.getString(AppConstants.DB_PASSWORD)
                    String DB_URL = BUNDLE.getString(AppConstants.DB_URL);
                    Class.forName(AppConstants.DRIVER_CLASS_NAME);
                    mConnection = DriverManager.getConnection(DB_URL, DB_USERNAME,
                            DB_PASSWORD);
                    mConnection.setAutoCommit(false);       
                }catch (NullPointerException e) {                   
                    throw new Exception(errorString+":"+e.getMessage());
                }
                catch(SQLException sqle) {
                    throw new SQLException(errorString+":"+sqle.toString());
                }


            if (mConnection == null) {
                throw new SQLException(errorString);
            } else {
                synchronized (lockObject) {
                    openCount++;                    
                }
            }
        }
        catch(Exception ex)
        {
                ex.printStackTrace();
throw new SQLException(e.getMessage());
        }
    }

    /** Return Connection **/
    public Connection getDBConnection()
    {
        return mConnection;
    }

    /**
     * Execute any plain SQL query and returns the ResultSet
     */
    public ResultSet executeSQL(String query)  throws SQLException
    {
        ResultSet lRSet = null;
        try
        {
            // close the statement if already open
            if(mStatement != null) mStatement.close();
            mStatement = mConnection.createStatement();
            lRSet = mStatement.executeQuery(query);
        }
        catch(SQLException ex)
        {
            System.err.println("Error Query : " + query);
            ex.printStackTrace();
                throw new SQLException(ex.getMessage());
        }

        return lRSet;
    }

    public void commit()
    {
        try
        {
            if(mStatement != null) mStatement.close();
            if(mConnection != null && !mConnection.getAutoCommit()) mConnection.commit();
        }
        catch(Exception ex)
        {
            ex.printStackTrace();
            throw new Exception(ex.getMessage());
        }
    }

    public void close()
    {
        try
        {
            if ((mConnection != null) && !mConnection.isClosed()) {
                if(!resultSetVector.isEmpty())
                {
                    //ok close all result sets once more
                    int rsSize = resultSetVector.size();
                    for(int i=0;i<rsSize;i++)
                    {
                        ResultSet rset=(ResultSet)resultSetVector.get(i);
                        try
                        {
                            rset.close();
                        }
                        catch(SQLException e)
                        {
                                throw new SQLException(e.getMessage());
                        }
                    }
                }

                if(!statementVector.isEmpty())
                {
                    //ok close all statements once more
                    int stmtSize = statementVector.size();
                    for(int i=0;i<stmtSize;i++)
                    {
                        CallableStatement cstmt=(CallableStatement)statementVector.get(i);
                        try
                        {
                            cstmt.close();
                        }
                        catch(SQLException e)
                        {
                            //ignore this
                        }
                    }
                }

                mConnection.close();
                synchronized (lockObject) {
                    closeCount++;                   
                }
            }
        }
        catch(Exception ex)
        {
            ex.printStackTrace();
               throw new Exception(ex.getMessage());
        }
    }




} //end of Rconnection

Below is the dao class

 import java.lang.reflect.Field;
import java.sql.CallableStatement;
import java.sql.Date;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Statement;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;



public class ReportDao {
    public RConnection connection = new RConnection();
    public enum ReportType{USERDETAILS_REPORT,SOMEOTHER_REPORT};
    public final static int NO_OF_RECORDS=6;

    public Map<Integer, TreeMap<String,String>> getDataForRepot(String startDate, String endDate,String reportType) throws Exception {
        BatchJLogger.logMessage(" Started Execution of method getDataForRepot " );
        ResultSet rs = null;
        Statement stmt = null;
        CallableStatement cstmt = null;
        try {
            stmt = connection.getDBConnection().createStatement();
            rs = getReportRecords(cstmt,startDate,endDate,reportType);
            Map<Integer, TreeMap<String,String>> values= getFormattedData(rs);
            BatchJLogger.logMessage(" No of records fetched  "+values.size() );
            BatchJLogger.logMessage(" End Execution of method getDataForRepot " );
            return values;

        }  finally {
            if (rs != null)
                rs.close();
            if (stmt != null)
                stmt.close();
            /*if(cstmt!=null){
                cstmt.close();
            }*/
            connection.close();
            BatchJLogger.logMessage(" End Execution of method getDataForRepot " );
        }
    }

    public ResultSet getReportRecords(CallableStatement cstmt,String startDate, String endDate,String reportType)
            throws SQLException,Exception {

        ResultSet rs = null;
        try{
            BatchJLogger.logMessage(" Started Execution of method getReportRecords ");

            String procedure = "{call SOME_PROC (?,?,?,?)}";
            cstmt = connection.createCallableStatement(procedure);
            int procId=7;
            switch (ReportType.valueOf(reportType)) {
            case USERDETAILS_REPORT:
                cstmt.setInt(1, 7);
                break;
            case SOMEOTHER_REPORT:
                cstmt.setInt(1, 8);
                break;
            default:
                break;
            }
            if(startDate!=null){
                cstmt.setTimestamp(2, BatchJUtil.convertToTimeStamp(startDate,true));
                cstmt.setTimestamp(3, BatchJUtil.convertToTimeStamp(endDate,false));
            }else{
                cstmt.setTimestamp(2, null);
                cstmt.setTimestamp(3, null);
            }
            cstmt.registerOutParameter(4,
                    getOracleParamReturnType("CURSOR"));
            cstmt.execute();
            rs = connection.getResultSet(cstmt, 4);
            BatchJLogger.logMessage(" End Execution of method getReportRecords ");
        }catch (Exception e) {
            e.printStackTrace();
            connection.close();
        }finally{
            return rs;  
        }


    }

    public static int getOracleParamReturnType(String paramName) {
        if (paramName == null)
            return -1;
        Field cursorField;
        try {
            Class c = Class.forName("oracle.jdbc.driver.OracleTypes");
            cursorField = c.getField(paramName);
            return cursorField.getInt(c);
        } catch (Throwable th) {
            th.printStackTrace();
            return -1;
        }
    }

    public Map<Integer, TreeMap<String,String>> getFormattedData(ResultSet rs ) throws SQLException, ParseException{
        Map<Integer, TreeMap<String,String>>  data = new TreeMap<Integer, TreeMap<String,String>>();

        List<String> coulmnNames = new ArrayList<String>();
        ResultSetMetaData rsmd = rs.getMetaData();
        int columnCount = rsmd.getColumnCount();
        for (int i = 1; i < columnCount + 1; i++ ) {
            coulmnNames.add(rsmd.getColumnName(i)) ;
        }
        int noOfRecords =0 ;
        while (rs.next()) {
            TreeMap<String,String> values = new TreeMap<String, String>();
            for (String name : coulmnNames) {
                if(name.equalsIgnoreCase("Submitted On")){
                    if(rs.getString(name)!=null){
                        String submittedOn =rs.getString(name);
                        values.put(name, submittedOn);  
                    }else{
                        values.put(name, null);
                    }

                }else{
                    values.put(name, rs.getString(name));   
                }

            }
            data.put(++noOfRecords, values);
        }
        return data;
    }

    public void deleteBatchJobRecord(String reportName) throws SQLException{
        try {
            String deleteRecord = "delete from BATCH where SOME_ID=? and RUN_ID=(Select MAX(RUN_ID) "
                + "from BATCH where SOME_ID=?)";
            PreparedStatement pstmt = connection.getDBConnection()
            .prepareStatement(deleteRecord);
            switch (ReportType.valueOf(reportName)) {
            case USERDETAILS_REPORT:
                pstmt.setInt(1, 7);
                pstmt.setInt(2, 7);
                break;
            case SOMEOTHER_REPORT:
                pstmt.setInt(1, 8);
                pstmt.setInt(2, 8);
                break;
            default:
                break;
            }
            int result = pstmt.executeUpdate();
            System.out.println(" record deleted " + result);
        } finally {
            connection.close();
        }

    }

}

Answer 1

1. Eating up exceptions

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

   Since your application cannot recover from most of the exceptions it should not catch and ignore (eat) exceptions but rather catch it, print the stack trace and pass it along.

2. NullPointerException

   String[] reportsList = args[0].split(",");
   

   When you do not provide any arguments to your application (args.length == 0 -> args[0] == NULL) it will throw a NullPointerException

3. finally block is not needed

   } finally {
       System.exit(0);
   }
   

   By default once the end of the main is reached a 0 return code is returned from the application, so this code is not needed.

4. Not passing the exception cause

   throw new SQLException("Can not create CallableStatement for sql ");
   

   When you wrap a caught exception with a new exception like in this case you should pass the caught exception into the new exception's constructor, otherwise you are loosing the actual cause of the exception.

5. Closing of JDBC objects not inside a finally block

   rs = getReportRecords(cstmt,reportType);
   while (rs.next()) {
       str = new String[6];
       for (int i = 0; i < 6; i++) {
           str[i] = rs.getString(i + 1);
       }
       vecDetails.addElement(str);
   }
   if (rs != null)
       rs.close();
   if (stmt != null)
       stmt.close();
   

   Your code does not guarantee that the ResultSet and the Statement objects will be always closed. close methods should be called from finally blocks.

6. Vector instead of ArrayList

   ArrayList should be preferred over Vector in non-multithreaded code.

7. Generics are not used

8. Unnecessary casting

   String str[] = (String[]) null;
   

9. Magic numbers

   str = new String[6];
   

   Usage of hardcoded magic numbers should be reduced as much as possible, in cases when it is not possible they should be defined as final static and given a meaningful name.

10. Catching of NullPointerException

    }catch (NullPointerException e) {                   
        throw new SQLException(errorString+":"+e.getMessage());
    }
    

    Your code should not be catching an unchecked NullPointerException (indicating a programming error) and throw a check SQLException (indicating a database problem).

---

This is not a complete list, I think only when these few problems are fixed we can start working on the design of this application.

You can use one of the tools such as:

• findbugs
• PMD
• checkstyle

to further improve the quality of your code

Answer 2

Adam already did a pretty good job, but here are some other comments:

1) the ResultSets are cached in resultSetVector. That could lead to a very serious memory leak. I must admit I have not read and understood everything, but it seems very strange to cache ResultSets.

2) Not using generics and using Vector's does look antiquated. However, this code might have been intended to be threadsafe and some of the Vector's might not be replaceable by List's. If RConnection was meant to be threasafe, it needs a look more work because it far from threadsafe, despite the Vector's and lockObject.