Is it possible to refactor this Java code?

Question

I have a simple code below:

import java.util.ArrayList;

public class BoidList extends ArrayList
{
  synchronized public Boid GetBoid( int idx_ ) throws ArrayIndexOutOfBoundsException
  {
    if( idx_ <  super.size() &&
        idx_ >= 0 )
    {
      return (Boid) super.get(idx_);
    }
    else
    {
      throw new ArrayIndexOutOfBoundsException();
    }
  }
  synchronized public void RemoveBoid( int idx_ ) throws ArrayIndexOutOfBoundsException
  {
    if( idx_ <  super.size() &&
        idx_ >= 0 )
    {
      super.remove(idx_);
    }
    else
    {
      throw new ArrayIndexOutOfBoundsException();
    }    
  }
}

There's a lot of similarity between the 2 methods, yet they do two different things. Is it possible to refactor this?

Answer 1

I think they are quite different, one gets an object by a specified index, the other removes it, no matter if there are code similarities the tasks that the methods perform are quite the opposite.

Answer 2

You could pull out the code that checks for validity into a separate method:

private void checkIndex(int i) throws ArrayIndexOutOfBoundsException
{
  if (idx < 0 || idx_ >= super.size()) {
    throw new ArrayIndexOutOfBoundsException();
  }
}

synchronized public void RemoveBoid( int idx_ ) throws ArrayIndexOutOfBoundsException
{
  checkIndex(idx_);
  super.remove(idx_);
}

Also, you would want to extend java.util.ArrayList[Boid].

But extending a java.util.ArrayList seems like a bad idea. Why do you want to do that?

Answer 3

You could write a function like the following:

private boolean checkIndex( int _idx ) throws ArrayIndexOutOfBoundsException {
    if ( idx_ <  super.size() && idx_ >= 0 ) {
         return true;
    } else {
         throw ArrayIndexOutOfBoundsException();
    }
}

And then use this method for your check. However this might only provide a small advantage over the code you already use.

What you definitely shouldn't do is try to make 1 method out of the 2 you already have. This would violate basically every OO-principle I know of ;-)

Answer 4

You can refactor it into a getOrRemoveBoid method where a parameter can handle the decision. But this looks like code-smell. The methods have different a semantic and should be seperated.

The most of the class is about exception handling and language syntax. You shouldn't mix the methods, they are DRY enough.

Answer 5

Just for your information, the ArrayList class has the get and remove methods which performs bounds check and throws an IndexOutofBoundsException if the specified index is out of the range of the list.

Unless the target platform is under Java 5, the use of generics may be a more preferable approach. Using generics, it would be possible to ensure that the contents of the ArrayList is all of the same type, which would take away the need to perform typecasts, type checks and face a possible ClassCastException.

For example:

List<String> list = new ArrayList<String>();
list.add("Hello");
list.add("World");
String s = list.remove(0);    // No need for typecasting.

list.add(10);                 // Compile error, as 10 is not of type String.

Answer 6

What is the real purpose of the BoidList? Consider the following:

List<Boid> boids = Collections.synchronizedList(new ArrayList<Boid>());

This line of code is more-or-less equivalent to the subclass you are attempting to create:

• Type-safety is enforced at compile-time
• ArrayList already throws an IndexOutOfBoundsException if the index is invalid
• Collections.synchronizedList() ensures synchronized access to the list

I don't see any reason to create your own subclass, based on the source code you provided.

Answer 7

Don't refactor just for the sake of it!

The only part where it's possible:

idx_ <  super.size() && idx_ >= 0

Would likely be a waste of time (stepping into the new "isValidIndex(int)" method might inflict a performance penalty versus the unmodified code if you do it many, many times).

Answer 8

public class BoidList extends ArrayList<Boid> {

    private void checkIndex(int idx) {
        if (idx >= super.size() || idx < 0) {
            throw new ArrayIndexOutOfBoundsException(String.valueOf(idx));
        }
    }

    synchronized public Boid getBoid(int idx) {
        checkIndex(idx);
        return super.get(idx);
    }

    synchronized public void removeBoid(int idx) {
        checkIndex(idx);
        super.remove(idx);
    }
}

this fixes your code in several ways:

• the checkIndex() method is what you want
• never declare to throws a RuntimeException since it is a programming error
• follow java naming conventions (GetBoid -> getBoid, RemoveBoid -> removeBoid)
• remove useless cast simply extending ArrayList

EDIT

you may also want to check the CopyOnWriteArrayList<Boid>

Answer 9

Why would you bother?

Less code is not a goal (except in a perl one liner contest :-) ).

Coding style should have three goals :- clarity, correctness and run time efficiency.

The original code is clear enough, it is presumably correct and will run as affeciently as anything using a java collection can.

Any attempt at refactoring would make both the class itself less clear, and, make any code using the class less clear. Any changes may introdce bugs so it will be less correct, and, it is unlikely to make a more efficient use of the array class.