patternjavaMinor
Trying to cut down redundancy in my code
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
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
Very often you do something like this:
This is a bubblesort algorithm. However, on some places you use the significantly more effective
Another pattern which I see repeated in your code is a loop to add items from one list to another:
You can get rid of a lot of code duplication by creating an interface and a method:
An example usage of this would be:
Many of your methods are both returning and modifying the
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.