HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Trying to cut down redundancy in my code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tryingcutredundancydowncode

Problem

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 organizableList = new ArrayList();

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

public ArrayList dateOldestToNewest() {
for(int i = 0; i dateNewestToOldest() {
for(int i = 0; i fromDateToDate(GregorianCalendar dayOne, GregorianCalendar dayTwo) {
ArrayList holder = new ArrayList();
for(int i = 0; i onDay(GregorianCalendar day) {
ArrayList newOrganizableList = new ArrayList();
for(int i = 0; i duringMonth(GregorianCalendar month) {
ArrayList newOrganizableList = new ArrayList();
for(int i = 0; i duringYear(GregorianCalendar year) {
ArrayList newOrganizableList = new ArrayList();
for(int i = 0; i expensesOnly() {
ArrayList newOrganizableList = new ArrayList();
for(int i = 0; i incomeOnly() {
ArrayList newOrganizableList = new ArrayList();
for(int i = 0; i byItemName() {
ArrayList sortableArrayList = expensesOnly();
Collections.sort(sortableArrayList, Expense.expenseNameComparator);
return sortableArrayList;
}

public ArrayList certainItem(String itemName) {
ArrayList sortableArrayList = expensesOnly();
ArrayList returnArrayList = new ArrayList();
for(int i = 0; i byQuantity() {
ArrayList sortableArrayList = expensesOnly();
Collections.sort(sortableArrayList);
return sortableArrayList;
}

public ArrayList certainQuantity(int certainQuantity) {
ArrayList

Solution

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

List someList = new ArrayList();


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 {
    /**
     * @param obj Element to check
     * @return True if element should be kept, false otherwise.
     */
    boolean shouldKeep(E obj);
}

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


An example usage of this would be:

List holder = addToAnotherList(organizableList, new FilterInterface() {
    @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.

Code Snippets

List<Something> someList = new ArrayList<Something>();
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);
        }
    }
}
for(int i = 0; i<organizableList.size(); i++) {
    Something g = organizableList.get(i)....;
    if(g.someCondition && g.otherCondition...) {
        anotherList.add(organizableList.get(i));
    }
}
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;
}
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;
    }
});

Context

StackExchange Code Review Q#36452, answer score: 5

Revisions (0)

No revisions yet.