11
\$\begingroup\$

I have recently designed a module which will do bulk upload for different contents. Basically the user will upload an Excel file, I will have to read and validate headers of excel then each value of cell for all columns. I have designed a framework please validate.

Below are the vo.

Template Header details: Excel for each content is defined as template which will have an ID and will have the header (Excel column header) for each template:

public class TemplateHeaderDetails extends BaseVO {
    Integer templateId;
    String templateName;
    public String headerNames;
    String mandatory;
    Integer fieldId;

    public Integer getTemplateId() {
        return templateId;
    }

    public void setTemplateId(Integer templateId) {
        this.templateId = templateId;
    }

    public String getTemplateName() {
        return templateName;
    }

    public void setTemplateName(String templateName) {
        this.templateName = templateName;
    }

    public String getHeaderNames() {
        return headerNames;
    }

    public void setHeaderNames(String headerNames) {
        this.headerNames = headerNames;
    }

    public String getMandatory() {
        return mandatory;
    }

    public void setMandatory(String mandatory) {
        this.mandatory = mandatory;
    }

    public Integer getFieldId() {
        return fieldId;
    }

    public void setFieldId(Integer fieldId) {
        this.fieldId = fieldId;
    }

}

Each header column is given a field Id as you can see in the vo above.

We have defined validation rules:

public class Rule extends BaseVO {

    String ruleName;
    Integer ruleId;
    String methodName;
    String className;
    String validationMessage;
    List<String> referenceFields;

    public String getRuleName() {
        return ruleName;
    }
    public void setRuleName(String ruleName) {
        this.ruleName = ruleName;
    }
    public Integer getRuleId() {
        return ruleId;
    }
    public void setRuleId(Integer ruleId) {
        this.ruleId = ruleId;
    }
    public String getMethodName() {
        return methodName;
    }
    public void setMethodName(String methodName) {
        this.methodName = methodName;
    }
    public String getClassName() {
        return className;
    }
    public void setClassName(String className) {
        this.className = className;
    }
    public String getValidationMessage() {
        return validationMessage;
    }
    public void setValidationMessage(String validationMessage) {
        this.validationMessage = validationMessage;
    }
    public List<String> getReferenceFields() {
        return referenceFields;
    }
    public void setReferenceFields(List<String> referenceFields) {
        this.referenceFields = referenceFields;
    }


}

Each Header column (Field id ) will be mapped to a rule

public class RulesMapping extends BaseVO {


    Integer fieldId;
    String referenceField;
    Integer ruleId;
    public Integer sequenceNumber;
    String ruleConfig;
    public Integer getRuleId() {
        return ruleId;
    }
    public void setRuleId(Integer ruleId) {
        this.ruleId = ruleId;
    }
    public  Integer getSequenceNumber() {
        return sequenceNumber;
    }
    public  void setSequenceNumber(Integer sequenceNumber) {
        this.sequenceNumber = sequenceNumber;
    }

    public Integer getFieldId() {
        return fieldId;
    }
    public void setFieldId(Integer fieldId) {
        this.fieldId = fieldId;
    }
    public String getReferenceField() {
        return referenceField;
    }
    public void setReferenceField(String referenceField) {
        this.referenceField = referenceField;
    }
    public String getRuleConfig() {
        return ruleConfig;
    }
    public void setRuleConfig(String ruleConfig) {
        this.ruleConfig = ruleConfig;
    }


}

Below is the code which will read the Excel file, validate and update the database and update excel with comments of validation messages and success.

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;

import org.apache.poi.ss.usermodel.Cell;
import org.apache.poi.ss.usermodel.DateUtil;
import org.apache.poi.ss.usermodel.RichTextString;
import org.apache.poi.ss.usermodel.Row;
import org.apache.poi.xssf.usermodel.XSSFSheet;
import org.apache.poi.xssf.usermodel.XSSFWorkbook;




public class BulkUploader {


    private static final String BULKUPLOAD_UPDATE_RECORD = "BULKUPLOAD_UPDATE_RECORD";

    // This will contain module name plus header details
    List<TemplateHeaderDetails>  details = new ArrayList<TemplateHeaderDetails>();

    // template Name to header details(contians column name ,mandatory columns , sequence)
    List<HeaderDetails> headerDetails =null;

    // all key value pair mapping (  holding column name and its respective value per row.
    Map<String,Object>  records =  new HashMap<String,Object>();


    // field ID to rules mapping
    Map<Integer,List<RulesMapping>>  rulesMapping =  new HashMap<Integer,List<RulesMapping>>();


    // Rule id to Rule 
    Map<Integer,Rule>  rules =  new HashMap<Integer,Rule>();


    Map<Integer,List<String>>  errorMessages =  new HashMap<Integer,List<String>>();

    // List of procedure to be executed
    List<SP>  procedureList = new ArrayList<SP>();




    public void setUpData(){



        Map<Integer,String>  spColumnNames =new  TreeMap<Integer,String>();
        spColumnNames.put(1,"BDATE");
        spColumnNames.put(2,"ACTION");
        spColumnNames.put(3,"BBULLETIN_ID");
        spColumnNames.put(4,"BBULLETIN_NO");
        spColumnNames.put(5,"DATATitle");
        spColumnNames.put(6,"DATAStatus");
        spColumnNames.put(7,"DATAIntroduction (Rewritten)");
        spColumnNames.put(8,"Summary");
        spColumnNames.put(9,"Direct Questions To");
        spColumnNames.put(10,"Effective Start Date");
        spColumnNames.put(11,"Tags/Keywords");
        spColumnNames.put(12,"Created Date");
        spColumnNames.put(13,"Creator Name");

        SP sp = new SP();
        sp.setProcedureName("POC_BULK_UPLOAD");
        sp.setColumnSeq(spColumnNames);

        procedureList.add(sp);

    }



    @SuppressWarnings("unchecked")
    public void readExcel() throws IOException, IllegalAccessException, InstantiationException, ClassNotFoundException, SecurityException, NoSuchMethodException, IllegalArgumentException, InvocationTargetException{
        FileInputStream file = new FileInputStream(new File("C:\\Data.xlsx"));

        //Get the workbook instance for XLS file
        XSSFWorkbook workbook = new XSSFWorkbook(file);

        //Get first sheet from the workbook
        XSSFSheet sheet = workbook.getSheetAt(0);

        // GET the header row
        Row headerRow = sheet.getRow(0);

        // LIst of headers from excel
        List<String> headers = new ArrayList<String>();
        Iterator<Cell> cells = headerRow.cellIterator();
        while (cells.hasNext()) {
            Cell cell = (Cell) cells.next();
            RichTextString value = cell.getRichStringCellValue();
            headers.add(value.getString());
        }

        // validate the template 
        Object[] headerValidation = validateTempalte(headers );

        // if validation fails then write back the message to user.
        if((Boolean) headerValidation[0]==false){
            List<String> headerValidationMsg = (List<String>) headerValidation[1];
            appendHeaderComments(workbook,sheet,headerValidationMsg,headers);
            return ;
        }

        // Assign a number to header.This is done so that when we reading excel value we can identiy the cell value belongs to which column 
        Map<String,Integer> headerSeqNumber = assignHeaderSeqNumber(headers);


        // 

        //Get iterator to all the rows in current sheet
        int rowNumber=0;
        Iterator<Row> rowIterator = sheet.iterator();
        while (rowIterator.hasNext()) {
            Row row = (Row) rowIterator.next();
            if(rowNumber==0){
                rowNumber++;
                continue;
            }
            System.out.println("Row no "+rowNumber);
            Iterator<String> columnsIterator = headerSeqNumber.keySet().iterator();
            while (columnsIterator.hasNext()) {
                String name = columnsIterator.next();
                System.out.println("Read columnName "+name);
                int cellType=row.getCell(headerSeqNumber.get(name)).getCellType();
                switch (cellType) {
                case Cell.CELL_TYPE_STRING:
                    RichTextString value= row.getCell(headerSeqNumber.get(name)).getRichStringCellValue();
                    String val=value!=null ?Utils.replaceNonAsciiChar(value.getString().trim()):null;
                    val=val.replaceAll(":", "");
                    val=val.replaceAll("'", "");
                    records.put(name,val);
                    break;
                case Cell.CELL_TYPE_NUMERIC:
                    if (DateUtil.isCellDateFormatted(row.getCell(headerSeqNumber.get(name)))) {
                        Date date = row.getCell(headerSeqNumber.get(name)).getDateCellValue(); 
                        records.put(name,Utils.convertDateToString(date, "dd-MMM-yyyy"));
                    }else{
                        double numericVal= row.getCell(headerSeqNumber.get(name)).getNumericCellValue();
                        records.put(name,numericVal);
                    }
                    break;
                case Cell.CELL_TYPE_BLANK:
                    System.err.println(" blank cell type ");
                    records.put(name,null);
                    break;
                default:
                    System.err.println(" NEither string no number "+row.getCell(headerSeqNumber.get(name)).getCellType());
                    System.err.println(" value "+row.getCell(headerSeqNumber.get(name)).getStringCellValue());
                    break;
                }

            }
            // once a row is read validate each cell 
            Object[]  validationResults=validateRecords(records);
            Boolean isDataValid =(Boolean) validationResults[0];

            List<String> vMessages =new ArrayList<String>();
            // if data is valid then update the value in db.
            if (isDataValid) {
                System.out.println(" Valid data sending for upating record");
                String msg = executeProcedure(records);
                vMessages.add(msg);
            }else{
                // if there is validation issues then add the messages to list.
                if(validationResults[1]!=null){
                    vMessages = (List<String>) validationResults[1];
                }
                System.err.println(" data is invalid");
            }
            errorMessages.put(rowNumber, vMessages);
            rowNumber++;
        }

        // append success or failue to excel.
         appendComments(sheet,headers);
         String filePath="C:\\Data.xlsx";
         FileOutputStream fileOut = new FileOutputStream(filePath);
         workbook.write(fileOut);
         fileOut.flush();
         fileOut.close();

    }



    @SuppressWarnings("unchecked")
    private Object[] validateRecords(Map<String,Object> recordValues) throws IllegalAccessException, InstantiationException, ClassNotFoundException, SecurityException, NoSuchMethodException, IllegalArgumentException, InvocationTargetException{

        Object[] values = new Object[2];
        List<String>  vMessages = new ArrayList<String>();
        Boolean isDataUploadedValid = true;
        Iterator<TemplateHeaderDetails> columnsIterator = details.iterator();
        while (columnsIterator.hasNext()) {
            TemplateHeaderDetails headerValue = columnsIterator.next();
            String columnName=headerValue.getHeaderNames();
            Object value= recordValues.get(columnName);
            //System.out.println(" columnName "+columnName);
            int fieldId = headerValue.getFieldId();
            List<RulesMapping> ruleList= rulesMapping.get(fieldId);
            //System.out.println(" ruleList "+ruleList);
            if(ruleList!=null){
                for (RulesMapping rulesMapping : ruleList) {
                     String configuration =rulesMapping.getRuleConfig();
                     int sequence =rulesMapping.sequenceNumber;
                     Integer ruleId = rulesMapping.getRuleId();
                     String config = rulesMapping.getRuleConfig();
                     Rule rule =  rules.get(ruleId);
                     String className=rule.getClassName();
                     String methodName=rule.getMethodName();
                     String message = rule.getValidationMessage();
                     List<String> referenceFields = rule.getReferenceFields();
                     Object obj=Class.forName(className).newInstance();
                     //Class<?>[] parameterTypes = Util.getParameterTypes( value,referenceFields,config,message,columnName );
                     Class<?>[] parameterTypes = new Class<?>[5];
                     parameterTypes[0]=Object.class;
                     parameterTypes[1]=List.class;
                     parameterTypes[2]=String.class;
                     parameterTypes[3]=String.class;
                     parameterTypes[4]=String.class;
                     Object[]  validationResults= (Object[]) obj.getClass().getMethod( methodName,parameterTypes).invoke(obj, value,referenceFields,config,message,columnName);
                     Boolean isValidData = (Boolean) validationResults[0];
                     if(!isValidData){
                         isDataUploadedValid=false;
                         vMessages.addAll((List<String>) validationResults[1]);
                     }
                }   
            }

        }
        values[0]=isDataUploadedValid;
        values[1]=vMessages;
        return values;
    }

    // checks if headers from excel matches the headers for template from db based on mandaotry filds.
    private Object[] validateTempalte(List<String> headers){
        Object[] values=new Object[2];
        Boolean isTempateProper=true;
        List<String> errorMessages = new ArrayList<String>();
        //List<HeaderDetails> headerDetails = (List<HeaderDetails>) details.getHeaderDetails();
        for (TemplateHeaderDetails headerDetail : details) {
            if(headerDetail.getHeaderNames().equalsIgnoreCase("ACTION") ||headerDetail.getHeaderNames().equalsIgnoreCase("DATAID")
                    ||headerDetail.getHeaderNames().equalsIgnoreCase("DATASTATUS ID") ||headerDetail.getHeaderNames().equalsIgnoreCase("Department Name")       
                    ||headerDetail.getHeaderNames().equalsIgnoreCase("Status of Record") ||headerDetail.getHeaderNames().equalsIgnoreCase("Description of the Status")
                    ||headerDetail.getHeaderNames().equalsIgnoreCase("Name of the Excel File") ||headerDetail.getHeaderNames().equalsIgnoreCase("Updated By")){
                    continue;
            }

            if(headerDetail.getMandatory().equalsIgnoreCase("Y") && !headers.contains(headerDetail.getHeaderNames())){
                isTempateProper=false;
                errorMessages.add(headerDetail.getHeaderNames() + " column is missing ");
            }
        }
        values[0]=isTempateProper;
        values[1]=errorMessages;
        return values;
    }



    private Map<String,Integer> assignHeaderSeqNumber(List<String> headers){
        Map<String,Integer> headerSeqNumber = new HashMap<String, Integer>();

        int i=0;
        for (String columnName : headers) {
              headerSeqNumber.put(columnName, i);
              i++;
        }

        return headerSeqNumber;

    }


    private String executeProcedure(Map<String, Object> recordValues) {
        try {
            for (SP procedure : procedureList) {
                String query = " call " + procedure.getProcedureName() + " ( ";
                Map<Integer, String> params = procedure.columnSeq;
                Iterator<Integer> keys = params.keySet().iterator();
                int i = 1;
                while (keys.hasNext()) {
                    Integer seq = (Integer) keys.next();
                    if (params.containsKey(seq)) {
                        String columnName = params.get(seq);
                        System.out.println(" columName used in SP "
                                + columnName);
                        if (columnName.equalsIgnoreCase("ACTION")) {
                            query += " 'I' ";
                        } else if (columnName.equalsIgnoreCase("BBULLETIN_ID")) {
                            query += " '1' ";
                        } else if (columnName
                                .equalsIgnoreCase("Department Name")) {
                            query += " 'Compliance' ";
                        } else {
                            query += " '" + recordValues.get(columnName) + "' ";
                        }
                    } else {
                        query += " null ";
                    }
                    if (i <= params.size() - 1) {
                        query += " ,";
                    }
                    i++;
                }
                query += " ,? ,?)";

                ServiceCaller.invokeService(BULKUPLOAD_UPDATE_RECORD, query);
                System.out.println(" complete query " + query);

            }
            return "SUCSSES";
        } catch (Exception e) {
            e.printStackTrace();
            System.err.println(" Errro while upadating record");
            return "FAILED";
        }
    }


    private void appendComments(XSSFSheet sheet ,List<String> headers){
        int rowNumber=0;
        Iterator<Row> rowIterator = sheet.iterator();
        while (rowIterator.hasNext()) {
            Row row = (Row) rowIterator.next();
            if(rowNumber==0){
                Cell headerCell = row.createCell(headers.size());
                headerCell.setCellValue("Comments");    
            }else{
                List<String> vMessages = errorMessages.get(rowNumber);
                StringBuilder builder = new StringBuilder("");
                for (String message : vMessages) {
                    builder.append(message);

                }
                Cell headerCell = row.createCell(headers.size());
                headerCell.setCellValue(builder.toString());    
            }
            rowNumber++;
        }

    }

    private void appendHeaderComments(XSSFWorkbook workbook ,XSSFSheet sheet ,List<String> messages,List<String> headers) throws IOException{

        Row headerRow = sheet.getRow(0);
        Cell headerCell = headerRow.createCell(headers.size());
        headerCell.setCellValue("Comments");

        Row firstRow = sheet.getRow(1);
        Cell commentCell = firstRow.createCell(headers.size());

        String finalMessage=null;
        for (String msg : messages) {
            finalMessage+=msg;
        }
        commentCell.setCellValue(finalMessage);

         String filePath="C:\\Data.xlsx";
         FileOutputStream fileOut = new FileOutputStream(filePath);
         workbook.write(fileOut);
         fileOut.flush();
         fileOut.close();

    }


    //  commnets field for each , combine messages from JAVA,DB.
    private void getRecrodStatus(){

    }



    public List<TemplateHeaderDetails> getDetails() {
        return details;
    }



    public void setDetails(List<TemplateHeaderDetails> details) {
        this.details = details;
    }



    public List<HeaderDetails> getHeaderDetails() {
        return headerDetails;
    }



    public void setHeaderDetails(List<HeaderDetails> headerDetails) {
        this.headerDetails = headerDetails;
    }



    public Map<String, Object> getRecords() {
        return records;
    }



    public void setRecords(Map<String, Object> records) {
        this.records = records;
    }



    public Map<Integer, List<RulesMapping>> getRulesMapping() {
        return rulesMapping;
    }



    public void setRulesMapping(Map<Integer, List<RulesMapping>> rulesMapping) {
        this.rulesMapping = rulesMapping;
    }



    public Map<Integer, Rule> getRules() {
        return rules;
    }



    public void setRules(Map<Integer, Rule> rules) {
        this.rules = rules;
    }



    public Map<Integer, List<String>> getErrorMessages() {
        return errorMessages;
    }



    public void setErrorMessages(Map<Integer, List<String>> errorMessages) {
        this.errorMessages = errorMessages;
    }



    public List<SP> getProcedureList() {
        return procedureList;
    }



    public void setProcedureList(List<SP> procedureList) {
        this.procedureList = procedureList;
    }




}

Please review design and implementation.

\$\endgroup\$
14
\$\begingroup\$

Concatenating strings

Sometimes you use a StringBuilder and sometimes the compound assignment operator (+=). The key to distinguish when to use what is this: if you know the amount of concatenations then you use +=; if you don't then you use a StringBuilder (or there are too many of them).

Exceptions

You're throwing a lot of them but you don't do anything with it. That's not proper error handling.

Paths

Your path to the excel worksheet is hardcoded, changing this to something the user can select (for example by passing it to the main arguments) would be helpful.

Iterators

You have the following inside your iterators:

Iterator<Cell> cells = headerRow.cellIterator();
while (cells.hasNext()) {
    Cell cell = (Cell) cells.next();

Since your Iterator is defined as Iterator<Cell>, I don't think you have to cast it Cell anymore.

Typing errors

  • validateTempalte
  • isTempateProper
  • SUCSSES
  • Errro while upadating record
  • getRecrodStatus

Self-defined types

In your validation methods you return something like this:

values[0]=isTempateProper;
values[1]=errorMessages;

This is very error prone (when I saw the result used, I didn't know why you suddenly casted the first value to a Boolean). Instead, create a simple class that holds those fields

class ValidationResult {
    private Boolean isTemplateProper;
    private List<String> errorMessages;
    // Getters + Constructor
}

Subsequently this will allow you to change

if((Boolean) headerValidation[0]==false)

to

if(!validationResult.isProperTemplate())

And you will get rid of the ugly Object[] return type.

Side-effects

Printing data from inside a method that isn't supposed to is a side-effect you don't want; let methods stick to their objective. Maybe it is a remainder of debugging?

Formatting

Add more spaces and whitelines! You should never have 30 lines of code without a single whiteline between them: it feels too cramped. Also remember to put spaces between compound statements "Row no "+rowNumber and method calls appendHeaderComments(workbook,sheet,headerValidationMsg,headers);.

Magic values

In readExcel() you're hardcoding the file location ("C:\\Data.xlsx) twice. Hardcoding in itself is questionable but this is even more prone to mistakes. Define it once and use it throughout the application.

Diamond operator

Sometimes you use it, sometimes you don't. Keep in mind that this

List<String> vMessages = new ArrayList<String>();

can be written as

List<String> vMessages = new ArrayList<>();

Intermediate variables

In validateRecords()'s inner loop you're declaring a variable for each rule.something() but you only use it once. That's 8 lines of code that aren't needed, definitely since you really just go from 2 words to 1 -> Very minor change.

And unless I'm looking past it: I see several variables defined there that are not used.

Looping amongst values

You're doing this a few times with different values:

headerDetail.getHeaderNames().equalsIgnoreCase("ACTION")

Consider using a collection which will allow easier handling:

String[] headersToSkip = new[] {"ACTION", "DATAID", "DATASTATUS ID", "Department Name" };

for(String header : headersToSkip) {
    if(header.equalsIgnoreCase(headerDetail.getHeaderNames())) {
        continue;
    }
}

While we're at it here: why does getHeaderNames() return a single String? Make it singular instead.

Naming

Make your methods and variables as descriptive as possible; often this means you don't abbreviate them. For example:

  • assignHeaderSeqNumber => assignHeaderSequenceNumber
  • params => parameters
  • columnSeq => columnSequence
\$\endgroup\$
  • \$\begingroup\$ Telling beginners that they "should never have 30 lines of code without a single whiteline" is dangerous — not only is it an exaggeration, but they usually have the opposite problem. (The OP had excessive blank lines until I edited it to remove them, although none of them were the sort you're asking for.) \$\endgroup\$ – Anonymous May 21 '14 at 7:15
  • 1
    \$\begingroup\$ You really, really shouldn't edit the code's formatting. It's one of the aspects that get reviewed; I would've been able to provide more specific guidance if you hadn't done that. \$\endgroup\$ – Jeroen Vannevel May 21 '14 at 11:13
  • \$\begingroup\$ All right, I reverted the formatting. \$\endgroup\$ – Anonymous May 21 '14 at 17:17
7
\$\begingroup\$

AFAICT this code is not about uploading; rather it's about importing data from Excel files into a database, after validating it with rules loaded from somewhere (possibly the same database).

I'm ignoring the debug prints, commented-out code and hardcoded filenames, since they look like artifacts of a work in progress.

Architecture

The first three classes have some peculiar problems:

  • They have no constructors.
  • They have setters and getters for every field. Most of these are probably unnecessary. Setters in particular are often useless, because many fields can't meaningfully change. (templateId, for example, can presumably never change.)
  • They have no other methods, even though parts of BulkUploader ought to be methods of Rule and RulesMapping.
  • They all derive from BaseVO, which appears to have nothing to do with their meanings.

None of this would ordinarily be good style. I'm guessing this is done so they can be stored by some database library which expects this style. In that case it may be a necessary evil, but it's still annoying.

Classes with "details" in their names are a bad sign. TemplateHeaderDetails apparently represents either a template (in which case it should be called Template) or the list of headers used by a template (in which case it should be called TemplateHeaders).

TemplateHeaderDetails.mandatory should be a boolean, not a "Y"/"N" string.

If Rule is only for validation rules, it should be called ValidationRule. "Rule" is so vague it's uninformative.

Writing the success or failure message to the Excel file is odd. The user probably won't see it there. It's better to leave the file unmodified and report the message to the user in some other way.

Bugs

Unescaped strings

executeProcedure doesn't escape strings when building the query. This means input containing quotes, newlines, and possibly other special characters will not be imported correctly, and may create an SQL injection vulnerability. readExcel tries to work around this by removing some characters, but this also corrupts the data.

Please test with data containing quotes, newlines, non-ASCII characters, etc. Make sure it's imported intact. Removing the characters is not correct.

Data from previous rows

If a column is not set in one row (e.g. because its cell type is unknown), the value from the previous row (and even a previous file!) remains. This is because BulkUploader.records is reused: every row is represented by the same map.

records is used only in readExcel, so it should be a local variable. It represents a single row, so it should be created anew for each row.

Simplifications

AFAICT BulkUploader.procedureList always has one element. So it should be an SP, not a List<SP>.

BulkUploader.headerDetails is unused.

Most of BulkUploader's accessors are probably unnecessary.

flush before close is redundant. Just call close. (Or use a try (...) block so it will be closed reliably.)

The validation methods currently return an array of {success, error message}. This doesn't make it clear that the input is invalid and the import can't continue — the caller has to remember to check the result. It would be clearer to have them throw exceptions on failure.

In validateRecords, configuration and sequence are unused.

Some methods should be factored out:

  • The part of readExcel that converts a row to entries in records should be a separate method (rowMap, called as Map<String, Object> records = rowMap(row);?). The part that converts a cell to an entry in records could also be a separate method.
  • The part of validateRecords that finds the Rule to use should be a method of RulesMapping.
  • The part of validateRecords that runs the rule should be a method of Rule.
  • The part of executeProcedure that translates a column value to a string (with special cases for some columns) should be a separate method (parameterString?).

The loop in executeProcedure could be a for loop that updates i, to make it clear that it counts iterations:

for (int i = 1; keys.hasNext(); ++i)

You can build arrays in one line using array initializers:

Class<?>[] parameterTypes = {Object.class, List.class, String.class, String.class, String.class};
Object[] values = {isDataUploadedValid, vMessages};

parameterTypes isn't even needed, because you can pass the classes directly to getMethod:

Method m = obj.getClass().getMethod(methodName, Object.class, List.class, String.class, String.class, String.class);

FileInputStream's constructor accepts strings, so there's no need to convert the string to a File. Just do new FileInputStream(filename).

\$\endgroup\$
  • \$\begingroup\$ Thanks for detailed analysis .I have query Why should i throw exception from validation would it not be better to have error message and boolean variable indicating failure or success. \$\endgroup\$ – VIckyb May 22 '14 at 14:59
  • \$\begingroup\$ Throwing an exception means the caller doesn't have to remember to check the result, and makes it obvious that it's an error, not just a function returning false. (It's also easier for the caller to figure out what to do with an exception than with an Object[].) This is not important, though. \$\endgroup\$ – Anonymous May 22 '14 at 16:24
6
\$\begingroup\$

Just some general thoughts.

Are your field package protected by purpose? Do prevent creating a do tight coupling of your components while development you should start in general with private and only increase the visibility where necessary. On the other hand if you "never" plan to add additional operations to your data object, make them public and skip the useless getters.

Are the comments only for us now, or are this your real comments?

// List of procedure to be executed
List<SP>  procedureList = new ArrayList<SP>();

In later case: Is this adding any value to the reader? Just find a suitable variable name, as you did here, and skip the comment.

Some of your method are pretty long, maybe you can extract some well name method to improve readability. Also reducing the nesting level that way, might make it easier to maintain the code.

\$\endgroup\$

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.