Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The below code does Read csv files and process it to do some sort of cacluations.This is more of small picture code , to introduce big picture. The csv file contains information of seeds of crop which have some information in number depending upon the numbers the calcuation have to be done.There are different types of calucations based on traits types. The csv file will contains hybrid numbers followed by the traits . I have recorded observations for 6 traits, now at first i will read traits used in the applicaitons some traits may have 2 ,3 or n enteries.This is stored in db so i will pull inforamtion from there , once i read traits then i will read each hybrid number and trait values once i read trait values of particular trait i will either avgerage or do some more processing for som e trait the value depends on other triats.

Please have a look at it and let me know if it has to be improveed from design/implementation point of view.

Hybrid  PLTHT01 PLTHT02 Avg YLDTH01 YLDTH01 YLD TLSSG01 TLSSG02 TLSSG03 TLSSV
HH01    24  42      23  33      42  34  22  
HH02    26  40      27  37      42  34  22  
HH03    28  38      31  41      42  34  22  
HH04    30  36      35  45      42  34  22  
HH05    32  34      39  49      42  34  22  
HH06    34  32      43  53      42  34  22  
HH07    36  30      47  57      42  34  22  
HH08    38  28      51  61      42  34  22  
HH09    40  26      55  65      42  34  22  
HH10    42  24      59  69      42  34  22  

I have even entered values for TLSSG01 TLSSG02 TLSSG03 but they are not visible ,

Below is the Code:

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

public class HybridDataProcessor {
    public static void main(String[] args) {
        BufferedReader br = null;

        try {

            // holds currentline vlues
            String sCurrentLine;

            Map<String, HashMap<String,Float>> hybridTraitValues = new HashMap<String,HashMap<String, Float>>();

            // a mapping to traitName and no of columns it will have
            HashMap<String, Integer> listDataPointMap = new HashMap<String, Integer>();
            listDataPointMap.put("PLTHT",2);
            listDataPointMap.put("YLDTH",2);
            listDataPointMap.put("YLD",1);
            listDataPointMap.put("TLSSG",3);
            listDataPointMap.put("TLSSV",1);
            List<String> traitList = new ArrayList<String>();
        //  HashMap<String, Float> traitValues = new HashMap<String, Float>();
            br = new BufferedReader(new FileReader("C:\\testing.csv"));
            int lineNumber = 0;
            while ((sCurrentLine = br.readLine()) != null) {
                lineNumber++;
                String[] itemValues = sCurrentLine.split(",");

                // if line number then we need to get all the tratis used in csv file 
                if (lineNumber == 1) {
                    for (int j = 1; j < itemValues.length; j++) {
                        String traits = itemValues[j];
                        // if column name is avg do not consider it as trait
                        if(traits.equalsIgnoreCase("avg")){
                            continue;
                        }


                        // since yld is special trait and it does not have 01 attached to it  we will add it directly 
                        if(traits.equalsIgnoreCase("YLD")){
                            traitList.add(traits);
                            continue;
                        }
                     // since TLSSV is special trait and it does not have 01 attached to it  we will add it directly
                        if(traits.equalsIgnoreCase("TLSSV")){
                            traitList.add(traits);
                            continue;
                        }


                        traits=traits.substring(0,traits.length()-2);
                        if(!traitList.contains(traits))
                        traitList.add(traits);
                    }

                } else {
                    String hybridName = itemValues[0];

                    int datPts=0;
                    int currentPosition = 1;
                    for (String traits : traitList) {
                        // calcutes the vlalue  of each trait with respective hybrid 
                        if(traits.equalsIgnoreCase("YLD")){
                            float avg =getYLD(hybridTraitValues.get(hybridName).get("YLDTH"));
                            update(hybridTraitValues, hybridName, avg, traits);
                            currentPosition=currentPosition+1;
                        } else if(traits.equalsIgnoreCase("TLSSV")){
                            float avg =gettlssv(hybridTraitValues.get(hybridName).get("TLSSG"));
                            update(hybridTraitValues, hybridName, avg, traits);
                            currentPosition=currentPosition+1;
                        }   else {
                            datPts=listDataPointMap.get(traits);
                            float avg =getAvg(itemValues, datPts, currentPosition);
                            update(hybridTraitValues, hybridName, avg, traits);
                            if(traits.equalsIgnoreCase("PLTHT")){
                                currentPosition=currentPosition+datPts+1;
                            }else{
                                currentPosition=currentPosition+datPts; 
                            }
                        }

                    }
                }

            }

        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            try {
                if (br != null)
                    br.close();
            } catch (IOException ex) {
                ex.printStackTrace();
            }
        }

    }

    public static float getAvg(String[] itemValues,int dataPts,int currentPosition){
        float avg=0;
        for (int j = currentPosition; j < (currentPosition + dataPts); j++) {
            avg+=Float.parseFloat(itemValues[j]);
        }
        return  avg=avg/2;
    }

    public static float getYLD(float avg){
        return  avg=avg/10;
    }

    public static float gettlssv(float avg){
        return  avg=avg/10;
    }

    public static void update(Map<String, HashMap<String,Float>> hybridTraitValues,String hybridName,float avg,String traitName){
        HashMap<String, Float> traitValues= hybridTraitValues.get(hybridName);
        if(traitValues==null){
            traitValues = new HashMap<String, Float>();
        }
        traitValues.put(traitName, avg);
        hybridTraitValues.put(hybridName, traitValues);
    }


}

Now what if no of traits increases let if i add 10 more traits with different calcuations, how will i maintain the code please help.Does this look like oo code.

share|improve this question
    
Don't reinvent the wheel. opencsv.sourceforge.net –  Dave Jarvis Jan 9 '13 at 19:01
add comment

2 Answers

  • The path to the CSV file can be a parameter, it should not be hard coded, the path separator is available.
  • Depending on the file size there are more modern options to read files. (Guava & File NIO )
  • Checking for null values before parsing.
  • Method names are camel case "gettlssv" can be "getLSSV" ?
share|improve this answer
add comment

I'm sorry to say, it doesn't really look like oo code.

You didn't follow the following base principles of object oriented design: (just some of them listed here)

  • Encapsulation / Separation of concerns: logical blocks of function should be grouped together and hide their implementation details. e.g. The class that calculates some values, then it should not implement the reading of a CSV file, as this could be encapsulated in another class. So the change of one class doesn't affect the other.
  • Objects - actually you don't really use objects. All the code is in one big class, which has the invocation code - the main method - in it.
  • Decoupling - everything in one class/method is the highest coupling out there.
  • Reuse - no code of this class can be reused, as it is all coded in this class.
  • Extensibility - all methods are static, so they can't be mocked out by clients or overriden by subclasses.

You should use common libraries for the CSV parsing, as this is an error prone task. The implementation in this code is in many ways an incomplete CSV parser.

I see the following components/classes in your system:

  • CliClient - the client code with the main method
  • CsvReader - encapsulates the CSV reading
  • CalculationHandler(s) - I dont fully see what you're doing, so this can result in more than one object
  • Domain objects - this are meaningful objects, that represent your Map - if they can be represented.

So I think you need to think more about the objects in your system. Which are needed? What are they doing? How do the objects relate and collaborate with each other?

share|improve this answer
    
THanks burna this is the sort of comment i was looking for , but i could learn if you could provide me ur version code for the above –  VIckyb Jan 10 '13 at 5:41
    
Please explain "Domain objects - this are meaningful objects, that represent your Map - if they can be represented." and why do we need encapsulate csv reading –  VIckyb Jan 10 '13 at 5:42
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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