5
\$\begingroup\$

I have created a class that has many methods for sorting an ArrayList of objects. Each method is somewhat similar in that they employ some kind of sort on an ArrayList and then return the arrayList. They are different in that some have different methods of comparison and they also call different get methods for the object in the ArrayList.

As I said it is kind of redundant, so there's a lot of code:

import java.util.Collections;
import java.util.GregorianCalendar;
import java.math.BigDecimal;
import java.util.ArrayList;

public class OrganizeRegister {

ArrayList<BudgetItem> organizableList = new ArrayList<BudgetItem>();

public OrganizeRegister(ArrayList<BudgetItem> organizableList) {
    this.organizableList = organizableList;
}

public ArrayList<BudgetItem> dateOldestToNewest()   {
    for(int i = 0; i<organizableList.size(); i++) {
        for(int j = 1; j<organizableList.size(); j++) {
            GregorianCalendar g1 = organizableList.get(j-1).getDateOfTransaction();
            GregorianCalendar g2 = organizableList.get(j).getDateOfTransaction();
            if(g1.after(g2)) {
                BudgetItem temp = organizableList.get(j-1);
                organizableList.set(j-1, organizableList.get(j));
                organizableList.set(j, temp);
            }
        }
    }
    return organizableList;
}

public ArrayList<BudgetItem> dateNewestToOldest() {
    for(int i = 0; i<organizableList.size(); i++) {
        for(int j = 1; j<organizableList.size(); j++) {
            GregorianCalendar g1 = organizableList.get(j-1).getDateOfTransaction();
            GregorianCalendar g2 = organizableList.get(j).getDateOfTransaction();
            if(g1.before(g2)) {
                BudgetItem temp = organizableList.get(j-1);
                organizableList.set(j-1, organizableList.get(j));
                organizableList.set(j, temp);
            }
        }
    }
    return organizableList;
}

public ArrayList<BudgetItem> fromDateToDate(GregorianCalendar dayOne, GregorianCalendar dayTwo) {
    ArrayList<BudgetItem> holder = new ArrayList<BudgetItem>();
    for(int i = 0; i<organizableList.size(); i++) {
        GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
        if(g.equals(dayOne)||g.equals(dayTwo)||(g.after(dayOne)&&g.before(dayTwo))) {
            holder.add(organizableList.get(i));
        }
    }
    organizableList = holder;
    for(int i = 0; i<organizableList.size(); i++) {
        for(int j = 1; j<organizableList.size(); j++) {
            GregorianCalendar g1 = organizableList.get(j-1).getDateOfTransaction();
            GregorianCalendar g2 = organizableList.get(j).getDateOfTransaction();
            if(g1.after(g2)) {
                BudgetItem temp = organizableList.get(j-1);
                organizableList.set(j-1, organizableList.get(j));
                organizableList.set(j, temp);
            }
        }
    }
    return organizableList;
}

public ArrayList<BudgetItem> onDay(GregorianCalendar day) {
    ArrayList<BudgetItem> newOrganizableList = new ArrayList<BudgetItem>();
    for(int i = 0; i<organizableList.size(); i++) {
        GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
        if(g.equals(day)) {
            newOrganizableList.add(organizableList.get(i));
        }
    }
    return newOrganizableList;
}

public ArrayList<BudgetItem> duringMonth(GregorianCalendar month) {
    ArrayList<BudgetItem> newOrganizableList = new ArrayList<BudgetItem>();
    for(int i = 0; i<organizableList.size(); i++) {
        GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
        if(g.MONTH == month.MONTH) {
            newOrganizableList.add(organizableList.get(i));
        }
    }
    return newOrganizableList;
}

public ArrayList<BudgetItem> duringYear(GregorianCalendar year) {
    ArrayList<BudgetItem> newOrganizableList = new ArrayList<BudgetItem>();
    for(int i = 0; i<organizableList.size(); i++) {
        GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
        if(year.YEAR == year.YEAR) {
            newOrganizableList.add(organizableList.get(i));
        }
    }
    return newOrganizableList;
}

public ArrayList<Expense> expensesOnly() {
    ArrayList<Expense> newOrganizableList = new ArrayList<Expense>();
    for(int i = 0; i<organizableList.size(); i++) {
        if(organizableList.get(i) instanceof Expense) {
            Expense e = (Expense) organizableList.get(i);
            newOrganizableList.add(e);
        }
    }
    return newOrganizableList;
}

public ArrayList<Income> incomeOnly() {
    ArrayList<Income> newOrganizableList = new ArrayList<Income>();
    for(int i = 0; i<organizableList.size(); i++) {
        if(organizableList.get(i) instanceof Expense) {
            Income e = (Income) organizableList.get(i);
            newOrganizableList.add(e);
        }
    }
    return newOrganizableList;
}

public ArrayList<Expense> byItemName() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    Collections.sort(sortableArrayList, Expense.expenseNameComparator);
    return sortableArrayList;
}

public ArrayList<Expense> certainItem(String itemName) {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    ArrayList<Expense> returnArrayList = new ArrayList<Expense>();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        if(sortableArrayList.get(i).getItemName().equalsIgnoreCase(itemName)) {
            returnArrayList.add(sortableArrayList.get(i));
        }
    }
    return returnArrayList;
}

public ArrayList<Expense> byQuantity() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    Collections.sort(sortableArrayList);
    return sortableArrayList;
}

public ArrayList<Expense> certainQuantity(int certainQuantity) {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    ArrayList<Expense> returnArrayList = new ArrayList<Expense>();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        if(sortableArrayList.get(i).getQuantity() == certainQuantity) {
            returnArrayList.add(sortableArrayList.get(i));
        }
    }
    return returnArrayList;
}

public ArrayList<Expense> byPlaceOfPurchase() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    Collections.sort(sortableArrayList, Expense.placeOfPurchaseComparator);
    return sortableArrayList;
}

public ArrayList<Expense> fromPlaceOfPurchase(String certainPlace) {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    ArrayList<Expense> returnArrayList = new ArrayList<Expense>();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        if(sortableArrayList.get(i).getPlaceOfPurchase().equalsIgnoreCase(certainPlace)) {
            returnArrayList.add(sortableArrayList.get(i));
        }
    }
    return returnArrayList;
}

public ArrayList<Expense> byMethodOfPay() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    Collections.sort(sortableArrayList, Expense.methodOfPayComparator);
    return sortableArrayList;
}

public ArrayList<Expense> certainMethodOfPay(String certainMethod) {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    ArrayList<Expense> returnArrayList = new ArrayList<Expense>();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        if(sortableArrayList.get(i).getMethodOfPay().equalsIgnoreCase(certainMethod)) {
            returnArrayList.add(sortableArrayList.get(i));
        }
    }
    return returnArrayList;
}

public ArrayList<Expense> priceHighestToLowest() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        for(int j = 1; j<sortableArrayList.size(); j++) {
            BigDecimal bd1 = sortableArrayList.get(j-1).getPrice();
            BigDecimal bd2 = sortableArrayList.get(j).getPrice();
            if(bd1.compareTo(bd2) == 1) {
                Expense temp = sortableArrayList.get(j-1);
                sortableArrayList.set(j-1, sortableArrayList.get(j));
                sortableArrayList.set(j,temp);
            }
        }
    }
    return sortableArrayList;
}

public ArrayList<Expense> priceLowestToHighest() {
    ArrayList<Expense> sortableArrayList = expensesOnly();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        for(int j = 1; j<sortableArrayList.size(); j++) {
            BigDecimal bd1 = sortableArrayList.get(j-1).getPrice();
            BigDecimal bd2 = sortableArrayList.get(j).getPrice();
            if(bd1.compareTo(bd2) == -1) {
                Expense temp = sortableArrayList.get(j-1);
                sortableArrayList.set(j-1, sortableArrayList.get(j));
                sortableArrayList.set(j,temp);
            }
        }
    }
    return sortableArrayList;
}

public ArrayList<Income> byOriginOfIncome() {
    ArrayList<Income> sortableArrayList = incomeOnly();
    Collections.sort(sortableArrayList, Income.originComparator);
    return sortableArrayList;
}

public ArrayList<Income> certainOrigin(String originName) {
    ArrayList<Income> sortableArrayList = incomeOnly();
    ArrayList<Income> returnArrayList = new ArrayList<Income>();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        if(sortableArrayList.get(i).getOriginOfIncome().equalsIgnoreCase(originName)) {
            returnArrayList.add(sortableArrayList.get(i));
        }
    }
    return returnArrayList;
}

public ArrayList<Income> byFormOfIncome() {
    ArrayList<Income> sortableArrayList = incomeOnly();
    Collections.sort(sortableArrayList, Income.formComparator);
    return sortableArrayList;
}

public ArrayList<Income> byGrossIncome() {
    ArrayList<Income> sortableArrayList = incomeOnly();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        for(int j = 1; j<sortableArrayList.size(); j++) {
            BigDecimal bd1 = sortableArrayList.get(j-1).getGrossIncome();
            BigDecimal bd2 = sortableArrayList.get(j).getGrossIncome();
            if(bd1.compareTo(bd2) == 1) {
                Income temp = sortableArrayList.get(i);
                sortableArrayList.set(j-1, sortableArrayList.get(j));
                sortableArrayList.set(j,temp);
            }
        }
    }
    return sortableArrayList;
}

public ArrayList<Income> byNetIncome() {
    ArrayList<Income> sortableArrayList = incomeOnly();
    for(int i = 0; i<sortableArrayList.size(); i++) {
        for(int j = 1; j<sortableArrayList.size(); j++) {
            BigDecimal bd1 = sortableArrayList.get(j-1).getNetIncome();
            BigDecimal bd2 = sortableArrayList.get(j).getNetIncome();
            if(bd1.compareTo(bd2) == 1) {
                Income temp = sortableArrayList.get(i);
                sortableArrayList.set(j-1, sortableArrayList.get(j));
                sortableArrayList.set(j,temp);
            }
        }
    }
    return sortableArrayList;
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I strongly doubt that some of your methods work the way you intended. certainItem for example. Do you get the return that you expected from that method? \$\endgroup\$ – Simon Forsberg Dec 1 '13 at 13:35
  • \$\begingroup\$ Okay, maybe I need more background. I did not want to flood you all with all of the classes, because there are about 5-6 different ones that are all pretty fleshed out. If you would like I will post them, and more information on how the program works as a whole. \$\endgroup\$ – comscis Dec 1 '13 at 13:43
  • \$\begingroup\$ As to answer the question from Simon: 1. User enters a string 2. Method takes the AL containing SuperClass items 3. It filters the SubClass objects we need into a new AL 4. It then loops through the new AL and gets the instance variable itemName 5. The method then compares it to the user entered string 6. Finally, it takes any objects that match that string and puts them into a return AL... and now I see your problem with it :) slight bug there \$\endgroup\$ – comscis Dec 1 '13 at 13:51
5
\$\begingroup\$

It is better to declare your variables as List<Something>, and instantiate with new ArrayList<Something>() to allow other implementations of List. Like this:

List<Something> someList = new ArrayList<Something>();

Very often you do something like this:

for(int i = 0; i<organizableList.size(); i++) {
    for(int j = 1; j<organizableList.size(); j++) {
        Something g1 = organizableList.get(j-1)...
        Something g2 = organizableList.get(j)...
        if(... some comparison between g1 and g2...) {
            // Switch positions of the two items
            BudgetItem temp = organizableList.get(j-1);
            organizableList.set(j-1, organizableList.get(j));
            organizableList.set(j, temp);
        }
    }
}

This is a bubblesort algorithm. However, on some places you use the significantly more effective Collections.sort method. I recommend writing Comparators for the different ways you want to sort by and then use Collections.sort.


Another pattern which I see repeated in your code is a loop to add items from one list to another:

for(int i = 0; i<organizableList.size(); i++) {
    Something g = organizableList.get(i)....;
    if(g.someCondition && g.otherCondition...) {
        anotherList.add(organizableList.get(i));
    }
}

You can get rid of a lot of code duplication by creating an interface and a method:

public interface FilterInterface<E> {
    /**
     * @param obj Element to check
     * @return True if element should be kept, false otherwise.
     */
    boolean shouldKeep(E obj);
}


public static List<Something> addToAnotherList(Collection<Something> list, FilterInterface<Something> filter) {
            List<Something> result = new ArrayList<Something>();
    for (Something something : list) {
                if (filter.shouldKeep(something))
                    result.add(something);
            }
            return result;
}

An example usage of this would be:

List<BudgetItem> holder = addToAnotherList(organizableList, new FilterInterface<BudgetItem>() {
    @Override
    public boolean shouldKeep(BudgetItem item) {
        GregorianCalendar g = item.getDateOfTransaction();
        if(g.equals(dayOne)||g.equals(dayTwo)||(g.after(dayOne)&&g.before(dayTwo)))
            return true;
        else return false;
    }
});

Many of your methods are both returning and modifying the organizableList array. This feels strange to me. It can be more preferable to create a copy of the list and sort it in a specific way and return that.

\$\endgroup\$
  • \$\begingroup\$ Would you mind posting how you would write a Comparator for objects? I just now learned how to write them, and I am not sure how they would work specifically for classes such as BigDecimal and GregorianCalendar. Other than that, thank you for the help! \$\endgroup\$ – comscis Dec 1 '13 at 14:45
3
\$\begingroup\$

Instead of doing the sort yourself it is much faster and simpler to use the builtin sort routine.

public class OrganizeRegister {
    static final Comparator<BudgetItem> AGE = new Comparator<BudgetItem>() {
        @Override
        public int compare(BudgetItem o1, BudgetItem o2) {
            return o1.getDateOfTransaction().compareTo(o2.getDateOfTransaction());
        }
    };

    final List<BudgetItem> organizableList;

    public OrganizeRegister(List<BudgetItem> organizableList) {
        this.organizableList = organizableList;
    }

    public List<BudgetItem> dateOldestToNewest() {
        Collections.sort(organizableList, AGE);
        return organizableList;
    }

    public List<BudgetItem> dateNewestToOldest() {
        Collections.sort(organizableList, Collections.reverseOrder(AGE));
        return organizableList;
    }

    public List<BudgetItem> fromDateToDate(GregorianCalendar dayOne, GregorianCalendar dayTwo) {
        List<BudgetItem> holder = new ArrayList<BudgetItem>();
        for (BudgetItem bi : organizableList) {
            GregorianCalendar dot = bi.getDateOfTransaction();
            if (dot.before(dayOne) || dot.after(dayTwo))
                continue;
                holder.add(bi);
        }
        Collections.sort(holder, AGE);
        return holder;
    }

    public List<BudgetItem> onDay(GregorianCalendar day) {
        List<BudgetItem> holder = new ArrayList<BudgetItem>();
        for (BudgetItem bi : organizableList) {
            GregorianCalendar dot = bi.getDateOfTransaction();
            if (dot.equals(day)) 
                holder.add(bi);
        }
        return holder;
    }

BTW GergorianCalendar is a very expensive object. Date most likely does what you want is much, much cheaper and smaller.

\$\endgroup\$
2
\$\begingroup\$

A few things which were not mentioned earlier:

  1. You have a bug here:

        GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
        if (g.MONTH == month.MONTH) {
            newOrganizableList.add(organizableList.get(i));
        }
    

    MONTH is a constant in the Calendar class:

    public abstract class Calendar implements Serializable, 
            Cloneable, Comparable<Calendar> {
    
        ...
        public final static int MONTH = 2;
        ...
    }
    

    Therefore, the condition will be always true. (There is a similar issue with YEAR in the next method.) Eclipse shows a warning for that.

    Furthermore, you might want to compare the year too, not just the month to avoid getting data from the same month but different year.

  2. Here I'd rename newOrganizableList to result to express the purpose of the variable:

    public ArrayList<BudgetItem> duringMonth(GregorianCalendar month) {
        ArrayList<BudgetItem> newOrganizableList = new ArrayList<BudgetItem>();
        for (int i = 0; i < organizableList.size(); i++) {
            GregorianCalendar g = organizableList.get(i).getDateOfTransaction();
            if (g.MONTH == month.MONTH) {
                newOrganizableList.add(organizableList.get(i));
            }
        }
        return newOrganizableList;
    }
    
  3. OrganizeRegister is not the best class name. From Clean Code, page 25:

    Classes and objects should have noun or noun phrase names like Customer, WikiPage, Account, and AddressParser. [...] A class name should not be a verb.

  4. This field could be private:

    ArrayList<BudgetItem> organizableList = new ArrayList<BudgetItem>();
    

    (Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.)

  5. You might want to create a defensive copy here:

    public OrganizeRegister(ArrayList<BudgetItem> organizableList) {
        this.organizableList = organizableList;
    }
    

    It would prohibit malicious clients to modify the internal data of the class and could save you a couple of hours of debugging. (Effective Java, 2nd Edition, Item 39: Make defensive copies when needed)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.