-1
\$\begingroup\$

I had already posted a question about the one I am asking right now.

To quickly revisit the scenarios:

I am trying to generate reports which involve fetching data from a DB, writing data to a CSV file, and sending email. I am pulling data for all the reports in the same format since I wanted to keep the DAO layer and CSV-writing independent of a particular report. The problem I am facing now is that most of the logic is the same for all reports. I still have different classes for different reports. Of course, there are some customizations there, but in different services.

ReportInterface where I have two methods:

/**
 * This interface should be implemented by the report programs.
 */

import java.io.IOException;
import java.sql.SQLException;
import java.text.ParseException;

import com.aig.dtc.report.batch.exception.SystemException;

public interface ReportsInterface {

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

    /**
     * called after execute. 
     */
    public String getMailContent(boolean singleDate,String startDate,String endDate) throws SystemException, ParseException;
}

RequestCallReport

import java.io.IOException;
import java.io.StringWriter;
import java.sql.SQLException;
import java.text.ParseException;
import java.util.Map;
import java.util.TreeMap;


public class RequestCallReport implements ReportsInterface {
    private static final String STILD = "~";
    private Map<Integer, TreeMap<String,String>> callMeReportData =null;
    String dateTimeStamp;


    public String[] execute(String startDate, String endDate,String  reportType) throws IOException,SQLException,Exception{
        String[] details= new String[5];
        ReportDao reportDao = new ReportDao();
        Object[] reportDetails = reportDao.getDataForRepot(startDate,
                endDate,reportType);
            callMeReportData = (Map<Integer, TreeMap<String,String>>) reportDetails[0];
            CsvWriter csvWriter = new CsvWriter();
            setCallMeReportData(callMeReportData);
            String[] fileName=csvWriter.writeDetailsToFile(reportType, callMeReportData,startDate,endDate);
            for (int i = 0; i < fileName.length; i++) {
                details[i]=fileName[i];
            }
            details[3]=(String)reportDetails[1];
            details[4]=(String)reportDetails[2];
            dateTimeStamp=fileName[2];
        return details;
    }

    public String getMailContent(boolean singleDate,String startDate,String endDate) throws SystemException, ParseException{
             // only this method has some diiferent logic in few reports
    }

    public Map<Integer, TreeMap<String, String>> getCallMeReportData() {
        return callMeReportData;
    }

    public void setCallMeReportData(
            Map<Integer, TreeMap<String, String>> callMeReportData) {
        this.callMeReportData = callMeReportData;
    }
}

RequestLeadReport

import java.io.IOException;
import java.io.StringWriter;
import java.sql.SQLException;
import java.text.ParseException;
import java.util.Map;
import java.util.TreeMap;

public class RequestLeadReport implements ReportsInterface {
    private static final String STILD = "~";
    private Map<Integer, TreeMap<String,String>> callMeReportData =null;
    String dateTimeStamp;

    public String[] execute(String startDate, String endDate,String  reportType) throws IOException,SQLException,Exception{
        String[] details= new String[5];
        ReportDao reportDao = new ReportDao();
        Object[] reportDetails = reportDao.getDataForRepot(startDate,
                endDate,reportType);
            callMeReportData = (Map<Integer, TreeMap<String,String>>) reportDetails[0];
            CsvWriter csvWriter = new CsvWriter();
            setCallMeReportData(callMeReportData);
            String[] fileName=csvWriter.writeDetailsToFile(reportType, callMeReportData,startDate,endDate);
            for (int i = 0; i < fileName.length; i++) {
                details[i]=fileName[i];
            }
            details[3]=(String)reportDetails[1];
            details[4]=(String)reportDetails[2];
            dateTimeStamp=fileName[2];
        return details;
    }

    public String getMailContent(boolean singleDate,String startDate,String endDate) throws SystemException, ParseException{
             // only this method has some diiferent logic in few reports
    }

    public Map<Integer, TreeMap<String, String>> getCallMeReportData() {
        return callMeReportData;
    }

    public void setCallMeReportData(
            Map<Integer, TreeMap<String, String>> callMeReportData) {
        this.callMeReportData = callMeReportData;
    }
}

I have at least 6 such reports. As of now, only getMailContent has changes but execution remains the same.

I have this code to fetch data from a DB. I am using the same code for all the reports.

ReportDao

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.sql.Timestamp;
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{CALLME_REPORT,LEADGEN_REPORT};
    public final static int NO_OF_RECORDS=6;

    public Object[] 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;
        Object[] details = new Object[3];
        try {
            stmt = connection.getDBConnection().createStatement();
            Object[] reportDetails = getReportRecords(cstmt,startDate,endDate,reportType);
            rs = (ResultSet)reportDetails[0];
            Map<Integer, TreeMap<String,String>> values= getFormattedData(rs);
            details[0]=values;
            details[1]=reportDetails[1];
            details[2]=reportDetails[2];
            BatchJLogger.logMessage(" No of records fetched  "+values.size() );
            BatchJLogger.logMessage(" End Execution of method getDataForRepot " );
            return details;

        }  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 Object[] getReportRecords(CallableStatement cstmt,String startDate, String endDate,String reportType)
            throws SQLException,Exception {
        Object[]  queryValues = new Object[3];
        ResultSet rs = null;
        try{
            BatchJLogger.logMessage(" Started Execution of method getReportRecords ");

            String procedure = "{call GET_DAILY_RPT_REC (?,?,?,?,?,?)}";
            cstmt = connection.createCallableStatement(procedure);
            int procId=7;
            switch (ReportType.valueOf(reportType)) {
            case REPORTCALL_REPORT:
                cstmt.setInt(1, 7);
                break;
            case REPORTLEAD_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.registerOutParameter(5,
                    java.sql.Types.VARCHAR);
            cstmt.registerOutParameter(6,
                    java.sql.Types.VARCHAR);
            cstmt.execute();
            rs = connection.getResultSet(cstmt, 4);
            System.out.println(" out params form proc");
            System.out.println(cstmt.getString(5));
            System.out.println(cstmt.getString(6));
            Timestamp cronJobStDate=cstmt.getTimestamp(6);
            String formattedDate=null;
            if(cronJobStDate!=null){
                formattedDate=cronJobStDate.toString();
                formattedDate=BatchJUtil.convertformat(formattedDate);
            }
            String changeInStDate=cstmt.getString(5);
            queryValues[0]=rs;
            queryValues[1]=changeInStDate;
            queryValues[2]=cstmt.getString(6);

            BatchJLogger.logMessage(" End Execution of method getReportRecords ");
            return queryValues;
        }catch (Exception e) {
            e.printStackTrace();
            connection.close();
        }

        return queryValues;


    }

    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 TBATCH_RUN_LOG where SYSTEM_ID=? and RUN_ID=(Select MAX(RUN_ID) "
                + "from TBATCH_RUN_LOG where SYSTEM_ID=?)";
            PreparedStatement pstmt = connection.getDBConnection()
            .prepareStatement(deleteRecord);
            switch (ReportType.valueOf(reportName)) {
            case REPORTCALL_REPORT:
                pstmt.setInt(1, 7);
                pstmt.setInt(2, 7);
                break;
            case REPORTLEAD_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();
        }
    }
}

CsvWriter

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 {

    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("Title");
        leadGen.add("First Name");
        leadGen.add("Last Name");
        leadGen.add("Mobile Phone");
        leadGen.add("Product Interested");
        leadGen.add("Submitted On");
        headers.put("CALLME_REPORT", callMe);
        headers.put("LEADGEN_REPORT", leadGen);
    }

    private FileWriter fileWriter = null;
    private String folderName = null;
    public enum ReportType{CALLME_REPORT,LEADGEN_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");
    }


    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 CALLME_REPORT:
                fileWriter.write("Call Me");
                break;
            case LEADGEN_REPORT:
                fileWriter.write("Lead Gen");
                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 CALLME_REPORT:
            fileName="reprot_call_"+toDaysDate+".csv";
            break;
        case LEADGEN_REPORT:
            fileName="reprot_lead_"+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();
        //cal.add(Calendar.DATE, -1);
        return dateFormat.format(cal.getTime());
    }

    public static String getPreviousDate(){
        DateFormat dateFormat = new SimpleDateFormat("dd-MMM-yyyy");
        Calendar cal = Calendar.getInstance();
        cal.add(Calendar.DATE, -1);
        return dateFormat.format(cal.getTime());
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why are your start and end date strings? Java has Date... Joda Time has even better (DateTime). \$\endgroup\$ – fge Jun 14 '13 at 9:01
  • \$\begingroup\$ they are passed as Strings arguments \$\endgroup\$ – VIckyb Jun 14 '13 at 9:50
  • \$\begingroup\$ Yes, but the question is why? \$\endgroup\$ – fge Jun 14 '13 at 9:55
  • \$\begingroup\$ it is passsed form command prompt,later i am converting to date format if there is better way let me know \$\endgroup\$ – VIckyb Jun 14 '13 at 11:19
  • 1
    \$\begingroup\$ The title of your post should be the function/purpose of your code. \$\endgroup\$ – SirPython Aug 4 '15 at 23:32
3
\$\begingroup\$

This code needs some refactoring. Here are my suggestions:

1) since you have some kinds of reports, which seem to share some common behaviour, you could use a base class and some classical inheritance. So you have to write the execution logic once for all cases, where no change is needed and override in your subclass, where changes are needed. And your interface would be obsolete.

"startDate", "endDate" and "reportType" would be member variables of the class. And at best start/endDate of some date-type and reportType as an enum.

2) it is bad to gereate new objects in objects. That leads to code, that is hard to test. Instead of

ReportDao reportDao = new ReportDao();

you could "inject" a Dao via "constructor injection". If you take step (1) you will have to split your concerns: a) you need an object to represent the report itself and b) you need an object, which creates different kinds of reports. There will be the right place to inject your DAO.

3) What are these details about?

String[] details= new String[5];

Will anybody know? Okay, I admit, at least you know. But think of a freshman, new to your code: it will take (too much) time, to get his head around the code.

4) Shorten your methods.

public Object[] getReportRecords(CallableStatement cstmt,String startDate, String endDate,String reportType)
                throws SQLException,Exception {
            Object[]  queryValues = new Object[3];
            ResultSet rs = null;
            try{
                BatchJLogger.logMessage(" Started Execution of method getReportRecords ");

                String procedure = "{call GET_DAILY_RPT_REC (?,?,?,?,?,?)}";
                cstmt = connection.createCallableStatement(procedure);
                int procId=7;
                switch (ReportType.valueOf(reportType)) {
                case REPORTCALL_REPORT:
                    cstmt.setInt(1, 7);
                    break;
                case REPORTLEAD_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.registerOutParameter(5,
                        java.sql.Types.VARCHAR);
                cstmt.registerOutParameter(6,
                        java.sql.Types.VARCHAR);
                cstmt.execute();
                rs = connection.getResultSet(cstmt, 4);
                System.out.println(" out params form proc");
                System.out.println(cstmt.getString(5));
                System.out.println(cstmt.getString(6));
                Timestamp cronJobStDate=cstmt.getTimestamp(6);
                String formattedDate=null;
                if(cronJobStDate!=null){
                    formattedDate=cronJobStDate.toString();
                    formattedDate=BatchJUtil.convertformat(formattedDate);
                }
                String changeInStDate=cstmt.getString(5);
                queryValues[0]=rs;
                queryValues[1]=changeInStDate;
                queryValues[2]=cstmt.getString(6);

                BatchJLogger.logMessage(" End Execution of method getReportRecords ");
                return queryValues;
            }catch (Exception e) {
                e.printStackTrace();
                connection.close();
            }

            return queryValues;


        }    

That is hard to read.

And unless your app is a console app:

System.out.println(" out params form proc");
System.out.println(cstmt.getString(5));
System.out.println(cstmt.getString(6));

there should be no reference to a console at all.

5) Perhaps it makes sense to outsource the writing of a report to a report writing class. There you have a List of Writer which take a Report as an input and produce the wanted output.

public class ReportWriter{
     public List<ReportOuptput> = new ArrayList<ReportOuptput>();
     // ...
     public void addOutputChannel(ReportOutput ... channels);
     // ...
     public void writeReport(Report report);
}

6) Use better naming: It is hard to understand, what something like

Map<Integer, TreeMap<String, String>>

is. Okay: A mapping of integer values to a treemap, which maps one string to another. Let's have a look deeper in the code:

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

It is "data"... And

TreeMap<String,String> values = new TreeMap<String, String>();

That are values. Up to this point, one has no idea, what this code is about. If you are dealing with books, you have a book, which consists of pages, which have lines, which have characters. So that would be the way to go.

7) What does the following code do?

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));   
    }

}

Effectively:

String submittedOn =rs.getString(name);
values.put(name, submittedOn);

values.put(name, null);

In case of "null", it puts "null" in there. And at least:

values.put(name, rs.getString(name));

So if I am getting it right: it puts, what it finds in values.

I think, you have a lot of work to do.

\$\endgroup\$
  • \$\begingroup\$ Thank you, thats the kind of review i needed, SInce i have same format for all the Reports still do i have to maintain different writers for each, can you also let me know if my data structure Map<Integer, TreeMap<String, String>> is this fine is there a better way \$\endgroup\$ – VIckyb Jun 17 '13 at 6:54
  • \$\begingroup\$ With all respect: I do not have a clue, what this structure expresses. (6) deals with that. As far as I got skimming your code was: a mapping of integer indices to column names and their values - which resembles something like a data grid. But that is only a guess. Your domain language is not clear about that. If I am guessing right, a List<Lines> or something would be more clear. \$\endgroup\$ – Thomas Junk Jun 17 '13 at 11:04
  • \$\begingroup\$ YOur right TreeMap<String, String> is mapping of column name and its value Map<Ineteger> is used to store rownumber so that i can presever order. so that i can iterate in ordererd manner and diplsay valuesn \$\endgroup\$ – VIckyb Jun 17 '13 at 11:33
  • \$\begingroup\$ I have one more question regarding u said it is bad to gereate new objects in objects does it apply only to dao , any objects. \$\endgroup\$ – VIckyb Jun 18 '13 at 13:31
  • \$\begingroup\$ No. It is a general principle. The following metaphor makes it clear: When building a car, no car takes care of building its engine. An engine is built into the car (from the outside so to say). So the car is flexible which engine could be used, as long one is implemented. \$\endgroup\$ – Thomas Junk Jun 18 '13 at 13:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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