Using generics
You've mentioned that you'd like to avoid typecasting and use generics instead. This is tricky, because a list shouldn't reveal it's implementation details, and making LockDList generic would do that; we can't just do
public class LockDList<T extends DListNode> extends DList {}
because now the user not only can know the internal mechanics, they must know. If we want to use generics, it has to be done internally with a factory. For example,
private class NodeFactory<T extends DListNode> {
private final Constructor<? extends T> ctor;
NodeFactory(Class<? extends T> impl) throws NoSuchMethodException {
this.ctor = impl.getConstructor();
}
protected T getNewNode(Object item, DListNode prev, DListNode next) throws Exception {
return ctor.newInstance(item, prev, next);
}
}
This is a little tricky. We have to have a Constructor object because Java uses type erasure , meaning that at runtime all Java knows about T is that it is some subclass of DListNode. This means that we also need all of this messy error handling in order to compile. Then in the body of our class, we can do something like this.
private NodeFactory<LockDListNode> factory;
{
try {
factory = new NodeFactory(Class.forName("LockDListNode"));
} catch (Exception cex) {
throw new RuntimeException("LockDListNode class not found");
}
}
Again, ugly error handling. But, at least we get to do this now!
protected LockDListNode newNode(Object item, DListNode prev, DListNode next) {
try {
return factory.getNewNode(item, prev, next);
} catch (Exception e) {
throw new RuntimeException("New node could not be created");
}
}
Except that is WAY worse than what you had before. It is much less legible and can throw new runtime errors. We also can't effectively (or at least I didn't come up with a way to) remove the casting from remove and lockNode (although because lockNode only applies to locked lists, you could probably make that take a LockDListNode).
The long and short of it is that you'll have to do a lot of generic hacking to remove 3 casts; casts that make sense and work. The generics don't make sense and might not work. And most importantly, because of type erasure , generics will use type casting under the hood anyway to ensure type safety.
Using generics
You've mentioned that you'd like to avoid typecasting and use generics instead. This is tricky, because a list shouldn't reveal it's implementation details, and making LockDList generic would do that; we can't just do
public class LockDList<T extends DListNode> extends DList {}
because now the user not only can know the internal mechanics, they must know. If we want to use generics, it has to be done internally with a factory. For example,
private class NodeFactory<T extends DListNode> {
private final Constructor<? extends T> ctor;
NodeFactory(Class<? extends T> impl) throws NoSuchMethodException {
this.ctor = impl.getConstructor();
}
protected T getNewNode(Object item, DListNode prev, DListNode next) throws Exception {
return ctor.newInstance(item, prev, next);
}
}
This is a little tricky. We have to have a Constructor object because Java uses type erasure , meaning that at runtime all Java knows about T is that it is some subclass of DListNode. This means that we also need all of this messy error handling in order to compile. Then in the body of our class, we can do something like this.
private NodeFactory<LockDListNode> factory;
{
try {
factory = new NodeFactory(Class.forName("LockDListNode"));
} catch (Exception cex) {
throw new RuntimeException("LockDListNode class not found");
}
}
Again, ugly error handling. But, at least we get to do this now!
protected LockDListNode newNode(Object item, DListNode prev, DListNode next) {
try {
return factory.getNewNode(item, prev, next);
} catch (Exception e) {
throw new RuntimeException("New node could not be created");
}
}
Except that is WAY worse than what you had before. It is much less legible and can throw new runtime errors. We also can't effectively (or at least I didn't come up with a way to) remove the casting from remove and lockNode (although because lockNode only applies to locked lists, you could probably make that take a LockDListNode).
The long and short of it is that you'll have to do a lot of generic hacking to remove 3 casts; casts that make sense and work. The generics don't make sense and might not work. And most importantly, because of type erasure , generics will use type casting under the hood anyway to ensure type safety.
Simplify your boolean logic.
In LockDList.remove you can remove two of the branches and end up with
if (node != null && !((LockDListNode)node).lock) {
node.item = null;
node.prev.next = node.next;
node.next.prev = node.prev;
this.size--;
}
Because we don't do anything in the other cases we don't need those empty returns, and we don't need two separate branches for node == null and ((LockDListNode)node).lock == true because the conditional will short circuit. Furthermore, you don't need to compare a boolean to true or false because it already is either true or false.
Similarly you can simplify lockNode. You don't need the empty conditional branch.
public void lockNode(DListNode node) {
if (node != null){
((LockDListNode) node).lock = true;
}
Use the supertype's implementation where possible
In remove you can simplify even further to
if (node != null && !((LockDListNode) node).lock) {
super.remove(node);
}
Override methods explicitly
I like to use the @Override annotation when I'm overriding a method. This provides the following benefits:
Indicates that a method declaration is intended to override a method declaration in a supertype. If a method is annotated with this annotation type compilers are required to generate an error message unless at least one of the following conditions hold:
- The method does override or implement a method declared in a supertype.
- The method has a signature that is override-equivalent to that of any public method declared in
Object.
As well as making it explicit to readers that you're overriding a method.
Things I noticed in DList
Unrelated to the LockDList logic, but I notice that you're somewhat inconsistent with your whitespace. I see things like
if (this.sentinel.next == sentinel){
return null;
}else{
return this.sentinel.next;
}
and
if(this.sentinel.prev == sentinel){
return null;
}else{
return this.sentinel.prev;
}
right next to each other. In general adding extra whitespace will make things easier to read; you can change those to something like
if (this.sentinel.next == sentinel) {
return null;
} else {
return this.sentinel.next;
}
I like whitespace between my curly braces and their surrounding statements, and on the outside of the parens in an if statement or while/for loop.
I also notice that your DList implementations of insert and remove also have the unnecessary empty conditional branches.