This discussion is archived
5 Replies Latest reply: Jan 20, 2013 3:18 PM by marlin RSS

Please advice for below code and implementation

814889 Newbie
Currently Being Moderated
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:
mport 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.
  • 1. Re: Please advice for below code and implementation
    gimbal2 Guru
    Currently Being Moderated
    Does this look like oo code.
    Before you ask for opinions you should first give your own. What do you think?
  • 2. Re: Please advice for below code and implementation
    814889 Newbie
    Currently Being Moderated
    it is not hence i wanted to know
  • 3. Re: Please advice for below code and implementation
    marlin Newbie
    Currently Being Moderated
    No this does not look like OO code

    Rule of thumb: Any fucntion that has more than about 7 lines is too messy so re-factor.
    Rule of thumb: If a code line is TOO long to display on the forum without haveing to scroll horizontally your code is absolutely unreadable (and so are your questions due to the absolutely lame formatting that is done on this forum) and no one is likely to help you.

    Well I must be really bored today so here goes.

    You need to learn to use classes to help you document and to reduce complexity.

    Consider your longest horizontal line of code.

    public static void update(Map<String, HashMap<String,Float>> hybridTraitValues,String hybridName,float avg,String traitName){

    This is unreadable.

    Why? because a data structure that is a map of a string to a map of a string to a float is too complex and too much information for a human to parse.

    That is why we wrap things in classes, to hide the complexity under another layer. This should be something more like

    public static void update(Traits hybridTraitValues, String hybridName, float avg, String traitName)

    if you wrapped your Map of a Map up in a class called Traits.

    Of course, as soon as you do that, you start to wonder why your update routine is a public static routine up in the main class. Shouldn't update be an object function applied to a single trait?

    I mean, instead of calling update(hybridTraitValues, hybridName, avg, traitName) shouldn't you be calling something like

    hybridTraitValues.get(hybridName).update(avg, traitName)

    Your main routine is way too long. Too much detail. Impossible to read. You need to reduce it. First of all factor out all the bufferedReader junk from the parsing of the lines

    while(...){
    String[] splitLIne = sCurrentLint.split(",");
    if(lineNumber == 1){
    parseFirstLine(splitLine...);
    } else {
    parseDataLine(splitLine...);
    }
    }

    Sorry for the look of the above code. I don't live on this forum, I haven't memorized the markup that they require for monospaced fonts, and they don't bother to list that option in the little plain text help window that I am looking at as I type this. WHAT A BAG!! Seriously, a froum dedicated to talking about code does not list the one single markup tag that actually matters in their help box. Does no one complain about this? I am now remembering why I abandoned this forum after Oracle picked it up.

    Anyway, the point being that a division like that allows you to split the fileReading from the details of extracting the data.

    Can you see in my code where you need to patch it if the second line is blank or if there is some screwy last line? Can you see the same thing easily in your code? What if you aren't reading from a file anymore - you want to split the file details from the data details.

    Your line parsing is garbage. Why? You are using a long pile of ifs to special case your special columns. Ugly, unseperable, unmaintainable. What have you really got? you have standard columns and special columns. Make them into classes.

    class StandardColumn implements Column{
    String name;
    ...
    public parseMyValue(value){...}
    }

    class NotStandareColumnType1 implements Column{
    String name
    ...
    public parseMyValue(value){..do special stuff here.}
    }

    Now you got a list of Columns (or a map collected by name) that can contain both the standard ones and the non-standard one. Parse away. Basically you want your names to map to objects and then those objects KNOWS what to do. Instead you are looking at the names in your main routine and making decisions about what to do using if statements at that high level. Polymorphism is allowing different classes, to do different things given the same name. As a result piles of conditionals (doing different things here and there) can generally be replaced by having piles of classes that each do slightly differen things. Same work but split up into smaller more manageable chunks.

    I got curious and looked at some of your other postings. You had one with code like this:

    else if(cropId.equalsIgnoreCase("TO") && traitName.equalsIgnoreCase("TLSSG_70")) {
    traitAvg=calculateTLCV(traitName, traitAvg,dataPoint, dataTraits, hybrid, repl, traitValues, dataPvalues,50);
    } else if(cropId.equalsIgnoreCase("TO") && traitName.equalsIgnoreCase("TLSSG_100")) {
    traitAvg=calculateTLCV(traitName, traitAvg,dataPoint, dataTraits, hybrid, repl, traitValues, dataPvalues,50);

    That is disgusting. You have elevated a detail (like ignoring the case) to be right in my eyes on every single line. One single helper function

    boolean ct(String c, String t){return cropId.equalsIgnoreCase(c) && traitName.equalsIgnoreCase(t);}

    allows you to change your code to:

    else if(ct("TO", "TLSSG_70") || ct("TO", "TLSSG_100")){
    traitAvg=calculateTLCV(traitName, traitAvg,dataPoint, dataTraits, hybrid, repl, traitValues, dataPvalues,50);
    }

    And the fact is, even that is the wrong way to do it. Instead of a huge list of ifs based on cropID and NameYou want something more like

    Calc calc = Calc.get(cropID, traitName); // get the calculation associated with this crop and trait
    calc.do(blah blah); // do whatever calcualtion is in the calc object.

    Yes, you have to build those calc objects somewhere and you had to load up a map that associated the right calcualtion with the right trait BUT you don't need to look at those details at the moment that you are doing the calcualtion. My code for doing the calculation is two lines long. Get the calcualtion out of some kind of map and then do it. Notice that I don't even require that you use an actual Map. That is a lower level detail managed in some other class. I just get the calc and do it. Can you tell that my code works (assuming that the lower level stuff works) at a glance? Can you make the same statement for your code?

    Basically your code is not extensible and not maintainable because you are using none of the features of OOP or even good programming to MANAGE and HIDE detail. You have one single long flat unreadable thing with irrelevant detail visible everywhere. Basically Little Tiny readable chunks - good, Big is Bad.

    Learn to think NOT in terms of code that works. Learn to think in terms of code that you can read. Call routines that don't even exist to hide details. Pretend that those routines exist. Can you read your code? If not rewrite it. AFTER you code looks good, AFTER you have code that you could hand to your MOTHER and have her read it and check that it makes sense - THEN you go implement the routines that make it happen. In your long flat code you are remembering where you are and what you have to do next and it is one long list of details that bury you and bury your MOM. Factor that into small routines. Delay all decisions that you possibly can to lower levels of your code. Try to make only one decision in each chunk of code. This is what allows your MOM to look at it and say, "Oh I see, if it is this way I do this otherwise I do that. That's pretty simple. Yes, that is right." The point is that you look at one routine, no more than 7 or so lines, and verify that it does the correct thing. You never need to understand more than 7 lines or so at one time. Divide and Conquer!


    Enjoy your rewrite!
  • 4. Re: Please advice for below code and implementation
    814889 Newbie
    Currently Being Moderated
    Great , this is the Sort of reply i always wanted .Great .


    I have some doubts if you could clear them it would be great .
    Can you please explain me more on this

    "What have you really got? you have standard columns and special columns. Make them into classes.

    class StandardColumn implements Column{
    String name;
    ...
    public parseMyValue(value){...}
    }

    class NotStandareColumnType1 implements Column{
    String name
    ...
    public parseMyValue(value){..do special stuff here.}
    }
    "
    How can i acutally implement it if you can some more inputs on this it would be great .

    One more doubt is on below
    "Consider your longest horizontal line of code.

    public static void update(Map<String, HashMap<String,Float>> hybridTraitValues,String hybridName,float avg,String traitName){

    This is unreadable.

    Why? because a data structure that is a map of a string to a map of a string to a float is too complex and too much information for a human to parse.

    That is why we wrap things in classes, to hide the complexity under another layer. This should be something more like

    public static void update(Traits hybridTraitValues, String hybridName, float avg, String traitName)

    if you wrapped your Map of a Map up in a class called Traits.
    "

    How should i define Triat Class.
  • 5. Re: Please advice for below code and implementation
    marlin Newbie
    Currently Being Moderated
    I can't very well help you rewrite your code, because I can't read your code. The lines are so long that I must horizontally scroll them to see both the start and the end. And they are so long vertically that the horizontal scroll bar is not visible when I look at the top of the file. And I am not going to copy the data, put it into my code editor and edit your code.

    I don't understand what you are doing and don't want to read a bunch of code to try to figure out what you are doing by seeing what the code does. You are probably in the wrong forum, Algorithms, but we're both here so I'll let that pass. You are not looking for Algorithm assistance but how to write code assistance which would be over in the newbie section.

    My comment about the class wrapping is essentially this:

    Your update code looks like this:

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


    If you wrap it in classes it could look like this:

    class TraitValues{
    private Map<String, Float> Values = new HashMap<String, Float>();

    public void add(String trait, Float value){Values.put(triat, value);}
    }

    class Hybrid{
    String name // a single hybrid has a name
    TraitValues values = new TraitValues(); // .. and a list of values

    static Map<String, Hybrid> all = new HashMap<String, Hybrid>();

    public Hybrid(String name){this.name = name; all.add(this);}

    public void add(String trait, Float value){values.add(trait, value);}

    public static Hybrid get(String name){
    String up name.toUpperCase();
    Hybrid h = all.get(up);
    if(h == null){h = new Hybrid(up);}
    return h;
    }
    }

    This allows you to create Hybrid objects that have a name and a list of values.
    Hybrid has a constructor that take a name, creates a blank values list, and adds the Hybrid to the static Map of all Hybrids. Hybrid has a static function, getHybrid, that will fetch a single Hybrid by name from the static list of all of them.

    In places that you used to call

    update(hybridTraitValues, hybridName, avg, traitName)

    You would call

    Hybrid.get(hybridName).add(traitName, avg);

    More intelligently, up where you have code that sets the hybridName;

    String hybridName = itemValues[0];

    you would get the Hybrid object that you are working with like this:

    Hybrid hy = Hybrid.get(itemValues[0]);

    and then in the places you used to call update you would have:

    hy.add(traitName, avg)

    You see, I have replaced your String that happens to be a hybrid name with a Hybrid class that maintains hybrid objects (that happen to have names). I have replaced your hybridTraitValues structure, with a static variable in the Hybrid class called Hybrid.all

    I did not do the same for your Traits, but I suspect that you want to do something similar like:

    class Trait{
    String name;
    Float value;

    ... I don't know what you want traits to do, possibly different calculations.
    }

    if You do that, you would need to fix the TraitValues class so that instead of being Maps from Strings to Floats it is a Map from Strings to Traits.

    You tend to be using Strings (like hybrid names and trait names) as keys to fetch values out of large structures and your code is littered with testing and fecthing using these String values. You can't abandon strings entirely because apparently, you have a text file with string values in it for your input data. But strings are stupid things. You want to map those Strings into objects, which can have code embedded which can be used to reduce your surface code.

    In nearly any code that looks like:

    if(str == "FOO"){
    .. bunch of code dealing with Foo
    } else if (str == "BAR"){
    ... bunch of code dealing with Bar
    } else if { str == "BAZ" ...

    You can probably do that with classes, creating classes FOO, BAR, BAZ, mapping your string to an object (like I did with Hybrid) and then calling code like:

    SomeObject so = SomeObject.get(str); // fetch out the appropriate type of object
    so.doSomething(); // let the object do the right thing

    And the doSomething is defined differently in your different classes.

    In your code I see you testing for strings like YLD, TLSSV and branching into different chunks of code. This suggests to me that you may need classes like YLD and TLSSV. I don't know what those are, ao I called them Columns and suggested code having a Column interface.

    If you are not familiar with static values and interfaces, I would recommend looking them up. They are an essential part of the OOP toolkit.

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points