Can this Java code be refactored?
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 are many similarities between the two methods, but they do two different things. Can this be refactored?
a source to share
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 announces a RuntimeException call, as it is a programming error.
- follow java naming conventions (GetBoid -> getBoid, RemoveBoid -> removeBoid)
- remove useless listing by simply expanding ArrayList
EDIT
you can also check CopyOnWriteArrayList <Boid>
a source to share
What's the real goal 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 trying to create:
- Type-safety is applied at compile time
-
ArrayList
already throwsIndexOutOfBoundsException
if the index is invalid -
Collections.synchronizedList()
provides synchronized access to the list
I see no reason to create my own subclass based on the source code you provided.
a source to share
Just for your information, the ArrayList
class has an get
and remove
that does bounds checking and throws IndexOutofBoundsException
if the specified index is out of range of the list.
If the target platform is under Java 5, using generics might be preferable. By using generics, it would be possible to ensure that the content ArrayList
is the same type, which saves you the trouble of doing type tricks, type checking, and colliding with the possible ClassCastException
.
For instance:
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.
a source to share
You can pull the validation code 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 like to extend java.util.ArrayList [Boid].
But extending java.util.ArrayList seems like a bad idea. Why would you want to do that?
a source to share
Why would you bother?
Less code is not the goal (except for the perl one liner contest :-)).
The coding style should have three goals: - clarity, correctness and efficiency.
The source code is clear enough, it is presumably correct, and will work just as well as anything the java collection might use.
Any refactoring attempt will make the class itself less understandable and make code without using the class less understandable. Any changes can introduce errors, so they will be less correct and unlikely to use the array class more efficiently.
a source to share
You can write a function like:
private boolean checkIndex( int _idx ) throws ArrayIndexOutOfBoundsException {
if ( idx_ < super.size() && idx_ >= 0 ) {
return true;
} else {
throw ArrayIndexOutOfBoundsException();
}
}
And then use this method to check. However, this can only provide a slight advantage over the code you are already using.
What you definitely don't need to do is try to make one method out of 2 that you already have. This will violate basically every OO principle I know of; -)
a source to share
You can refactor it to getOrRemoveBoid method where the parameter can handle the decision. But it looks like a code smell. Methods have different semantics and should be separated.
Most of the class is related to exception handling and language syntax. You don't have to mix the methods, they are DRY enough.
a source to share