# Review algorithm of this code

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

-
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
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
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
are the above two line which i commented you mean line 3,4 – VIckyb Jan 17 '13 at 6:51
last two line in my code? – VIckyb Jan 17 '13 at 6:52

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

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.

-