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.

In this piece of code i have to display some set of

  • data in serpentine order.In an experiment there are Replication,range,plot
  • Replications contain range,plot.A range contains plot.
  • if there is 2 replication 4 ranges 8 plots
  • then each replication contains 2 ranges.Each range has 2 plot so
  • data to displayed in serpentine order will be like this

          Replication     Range Plot
       1                  1    1
       1                  1    2
       1                  2    2
       1                  2    1
       2                  3    1
       2                  3    2
       2                  4    2
       2                  4    1
    

The actual data i have to display is something like this

Replication     Range Plot   Hybrid pedigree qty
           1                  1    1    HH1    HH1/HH1   1
           1                  1    2    HH2    HH2/HH2   1
           1                  2    2    HH3    HH3/HH3   3
           1                  2    1    HH4    HH4/HH4   4
           2                  3    1    HH1    HH1/HH1   1
           2                  3    2    HH2    HH2/HH2   1
           2                  4    2    HH3    HH3/HH3   3
           2                  4    1    HH4    HH4/HH4   4

but In DB data is stored scateredly, in order to display i have to do iteration .The serpentine data is stored is this format HashMap<Integer, HashMap<Integer, ArrayList<Integer>>>, where as hybrid information along with pedigree ,replication,range,plot,qty is stored in VO Following is the class for this:

public class HybridVo {
    private String hybridId;
    private String pedigree;        
    private int replicaiton;
    private int range;
    private int plot;
    private String qty;

    public String getHybridId() {
            return hybridId;
    }

    public void setHybridId(String hybridId) {
            this.hybridId = hybridId;
    }

    public String getPedigree() {
            return pedigree;
    }

    public void setPedigree(String pedigree) {
            this.pedigree = pedigree;
    }

    public int getReplicaiton() {
            return replicaiton;
    }

    public void setReplicaiton(int replicaiton) {
            this.replicaiton = replicaiton;
    }

    public int getRange() {
            return range;
    }

    public void setRange(int range) {
            this.range = range;
    }

    public int getPlot() {
            return plot;
    }

    public void setPlot(int plot) {
            this.plot = plot;
    }

    public String getQty() {
            return qty;
    }

    public void setQty(String qty) {
            this.qty = qty;
    }

}

Following is the code where logic resides

import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.TreeMap;

public class DisplayLogic {
    public static void main(String[] args) {
        // Lets assume that both the below variable are filled with data.
        TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>> serpentine = new TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>>();
        List<HybridVo>  hybridList = new ArrayList<HybridVo>();

        // first iterate over the  serpentine  variable to diplay serpentine row.
        Iterator<Integer> serIt = serpentine.keySet().iterator();
        while (serIt.hasNext()) {
            Integer replication = (Integer) serIt.next();
            TreeMap<Integer, ArrayList<Integer>> range = (TreeMap<Integer, ArrayList<Integer>>) serpentine
            .get(replication);
            Iterator ranIt = range.keySet().iterator();
            int z=0;
            while (ranIt.hasNext()) {
                z++;
                List mapKeys = null;
                Integer rangeVal = (Integer) ranIt.next();
                ArrayList<Integer> intData = range.get(rangeVal);
                mapKeys = intData;
                /*
                 * the following is the code to display hybrid in serpentine order
                 */
                 if (z % 2 == 0) {
                     Collections.sort(mapKeys);
                     Collections.reverse(mapKeys);
                 } else {
                     Collections.sort(mapKeys);

                 }

                 Iterator mainIt = mapKeys.iterator();
                 while (mainIt.hasNext()) {
                     Integer plot = (Integer) mainIt.next();
                     // we have to repeat this  iteration for each row of serpentine 
                     //  which to costly
                     Iterator<HybridVo> hybridVoIt = hybridList.iterator();
                     while (hybridVoIt.hasNext()) {
                         HybridVo hybridVo = (HybridVo) hybridVoIt.next();
                         // this is logic where we get hybrid value which matches as per 
                         // serpentine order 
                         if (hybridVo.getReplicaiton() == replication
                                 && hybridVo.getPlot() == plot
                                 && hybridVo.getRange() == rangeVal) {
                         }
                     }

                 }

            }
        }

    }
}

As you can see that the display algorithm will result into too many iterations which too bad specifically the hybrid iterator has to be iterated for each iteration of serpentine row.Can anybody think of better design/pattern/oop/algoritm

share|improve this question
    
Are the two last lines in the correct order in your example ? Same question for lines 3 & 4 ? –  pgras Jan 16 '13 at 13:52
    
You have a 0% accept rate. You should accept (and upvote) answers to your previous questions before continuing to ask any more. –  seand Jan 16 '13 at 16:42
    
Ever heard about the for-each loop? I see no reasons to use iterators explicitly. It makes your code a lot more complicated than it needs to be. –  codesparkle Jan 16 '13 at 17:16
    
I am sorry about accept rate i usuall send thank you in comment ok i will follow that from now on –  VIckyb Jan 17 '13 at 6:48
    
TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>> serpentine = new TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>>(); List<HybridVo> hybridList = new ArrayList<HybridVo>(); –  VIckyb Jan 17 '13 at 6:51

1 Answer 1

up vote 1 down vote accepted

This is not a typical codereview question. It is not about reviewing code for code quality, it is asking for an algorithm improvement.

But anyway, here we go:

serpentine order

I did not found any explanation on google what this is exactly.

As you can see that the display algorithm will result into too many iterations which too bad specifically the hybrid iterator has to be iterated for each iteration of serpentine row.Can anybody think of better design/pattern/oop/algoritm

Well, you just need the element with the suitable replication, plot and rangeval. So one thing you could do to get rid of this loop is to put all HybridVo in a HashMap and use replication, plot and rangeval as a (unique!) key and access them in this way.

Example (I have cleaned up the source a bit, too):

public class DisplayLogic {
    public static void main(final String[] args) {
        // Lets assume that both the below variable are filled with data.
        final TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>> serpentine = new TreeMap<Integer, TreeMap<Integer, ArrayList<Integer>>>();
        final Map<String, HybridVo> mapForHybridVo = new HashMap<>();

        // first iterate over the  serpentine  variable to display serpentine row.
        for (final Integer replication : serpentine.keySet()) {
            final TreeMap<Integer, ArrayList<Integer>> range = serpentine.get(replication);
            boolean reverseIntData = true;

            for (final Integer rangeVal : range.keySet()) {
                final ArrayList<Integer> intData = range.get(rangeVal);
                // the following is the code to display hybrid in serpentine order
                if (reverseIntData)
                    Collections.sort(intData, comparatorIntReverse);
                else
                    Collections.sort(intData);
                reverseIntData = !reverseIntData;

                for (final Integer plot : intData) {
                    final HybridVo hybridVo = mapForHybridVo.get(getHashForReplicationRangePlot(replication, rangeVal, plot));
                    if (hybridVo != null) {
                        // start here
                    }
                }
            }
        }
    }

    private static Comparator<Integer> comparatorIntReverse = new Comparator<Integer>() {
        @Override
        public int compare(Integer arg0, Integer arg1) {
            return -Integer.compare(arg0, arg1);
        }
    };

    static String getHashForReplicationRangePlot(final int replication, final int range, final int plot) {
        return replication + "#" + range + "#" + plot; //hash must be unique for different inputs
    }
}

I would further suggest to use better names and submethods to reduce the brackets depth.

share|improve this answer

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.