Review code for optimization and implementation

Question

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.

• Dave Jarvis: Don't reinvent the wheel. opencsv.sourceforge.net

Answer 1

• 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" ?

Answer 2

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?

• VIckyb: 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: 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