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.

I have a model class that contains 3 ArrayList which are in order by parallel of the same size. <Object><Calendar><Long> I want to sort it by the <Long> Is this the most clean? is there a better way? This doesn't seem memory efficient.

    public class ResultModel{
        private ArrayList<Object> sets = new ArrayList<Object>();
        private ArrayList<Calendar> dates=new ArrayList<Calendar>();
        private ArrayList<Long> unixtimes=new ArrayList<Long>();
    private class WinningSet implements Comparable<WinningSet>{
            private long unixtime;
            private Object set;
            private Calendar date;
            WinningSet(Object set,Calendar date,long unixtime){
                this.unixtime=unixtime;
                this.set=set;
                this.date=date;
            }
            @Override
            public int compareTo(WinningSet another) {
                return (int) (this.unixtime-another.getUnixtime());
            }
            public long getUnixtime() {
                return unixtime;
            }
            public Object getSet() {
                return set;
            }
            public Calendar getDate() {
                return date;
            }
        }
        public void sortSelf(){
            ArrayList<WinningSet> list=new ArrayList<WinningSet>();
            for(int i=0;i<this.unixtimes.size();i++){
                list.add(new WinningSet(this.sets.get(i),dates.get(i),unixtimes.get(i)));
            }
            Collections.sort(list,Collections.reverseOrder());
            demap(list);
        }
        private void demap(ArrayList<WinningSet> list) {
             ArrayList<Object> tSets = new ArrayList<Object>();
             ArrayList<Calendar> tDates=new ArrayList<Calendar>();
             ArrayList<Long> tUnixtimes=new ArrayList<Long>();
            for(WinningSet temp:list){
                tDates.add(temp.getDate());
                tSets.add(temp.getSet());
                tUnixtimes.add(temp.getUnixtime());
            }
            this.sets=tSets;
            this.dates=tDates;
            this.unixtimes=tUnixtimes;
        }
}
share|improve this question
1  
@palacsint thanks for pointing it out. –  wtsang02 Jan 16 '13 at 22:08
add comment

1 Answer 1

up vote 1 down vote accepted

1. Correctnes.

compareTo() method of the WinningSet class is not correct. As of now Integer.MAX_VALUE < System.currentTimeMillis(). Consider the following example:

    long time1 = System.currentTimeMillis();
    System.out.println(time1 > 3*Integer.MAX_VALUE);
    System.out.println(time1 - 3*Integer.MAX_VALUE);
    System.out.println((int)(time1 - 3*Integer.MAX_VALUE));

It could be the case that your events are generated around the same time and it will not result in any error but in general - e.g. in future, if you restore some serialized objects, it could lead to a possible problem. You can rewrite it int the following way:

        @Override
        public int compareTo(WinningSet another) {
            if (this.unixtime > another.getUnixtime()) return 1;
            if (this.unixtime < another.getUnixTime()) return -1;
            return 0;
        }

2. Performance

Copy of the 3 lists is indeed a bad thing. You can use set() method from ListIterator to traverse through the lists and replace values in place.

private void demap(ArrayList<WinningSet> list) {         
            Iterator li = list.iterator();
            Iterator si = sets.iterator();
            Iterator di = dates.iterator();
            Iterator ui = unixtimes.iterator();
            //you can add iterators for other lists here. 
            while (li.hasNext()) {// we assume number is the same
              WinningSet temp = li.next();
              si.next(); //need to advance to the right element;  
              di.next(); //need to advance to the right element;  
              ci.next(); //need to advance to the right element;  
              si.set(temp.getSet());
              di.set(temp.getDate());
              ui.set(temp.getUnixTime());
            }
        }

3. Design

Are you sure that instead of a 3 lists you can't consider TreeMap<Long, SomeClass> where SomeClass is pair or Calendar and Object ? In this case you will always have the right order of items to work with - there is no need to sort lists on demand and do an extra work.

share|improve this answer
    
I agree with point 2 and 3, but I am not understanding 1. If the two unixtime is very close then it should give a small Integer (less than 10 secs) . So how does Integer.MAX_VALUE have anything to do with this. Thanks. –  wtsang02 Jan 17 '13 at 20:26
    
@wtsang02, if you have 100% guarantee that they are close and will always be this close - it's not a problem. However, if by some accident you decide to compare two WinningSets by Unix time (month or so apart, if I recall correctly) you could have a problem. For example, if you decide in future to accumulate them somewhere for a long time then this way of comparison can result in a wrong behaviour. I just always prefer a usual way to write comparison just not to think about such cases. –  Andrey Taptunov Jan 18 '13 at 3:18
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.