Skip to main content
Code Review

Return to Revisions

5 of 6
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/

While Marc-Andre already talked a lot about Java Style Conventions, here's my 2 cents on your implementation.

(削除) Inheriting:

You did create a List. In Java it's customary to implement Interfaces if you have classes with similar use-cases and methods.

public class list<T> {

This should/could be:

public class List<T> implements java.util.List<T> {

In fact (削除ここまで) ##Naming:

I suggest you change the name of your implementation to avoid confustion. Java already comes with two lists ((interface)java.util.List and (class)java.awt.List), that are just named List, you don't really need to introduce a third one ;)

(削除) By the way, if you implement an interface you are required to Override the methods that are specified by it.

In the case of List that's quite a few, including but not restricted to:

public int size();
public boolean isEmpty();
public boolean contains(T item);
public void add(T item);
public void remove(T item);

When implementing them make sure to use the @Override annotation:

@Override
public void add(T item) {
 Node<T> oldHead = this.head;
 this.head = new Node<T>(item);
 head.setNext(oldHead);
 oldHead.setPrev(head);
 size++;
}

Which brings me to my next point: (削除ここまで)

##Hiding inner classes:

There is absolutely no need to show how your list works internally by exposing a node-class to all who got your List.

Instead in Java you can use a feature called 'Inner Classes'. It's quite simple:

public class MyList<T> implements List<T> {
 private Node<T> head = null;
 private int size = 0; //you could also use long when you expect more than 2150kk items
 public MyList() {}
 private static class Node<T> {
 private final T value;
 private Node<T> next = null;
 private Node<T> prev = null;
 protected Node<T>(T value) {
 this.key = value;
 }
 T value() {
 return this.value;
 }
 void setNext(Node<T> newNext) {
 this.next = newNext;
 }
 void setPrev(Node<T> newPrev) {
 this.prev = newPrev;
 }
 Node<T> getNext() {
 return this.next;
 }
 Node<T> getPrev() {
 return this.prev;
 }
 }
 //Here goes the rest of MyList implementation
}

I am using a few tricks here:
The first thing you might probably notice is the keyword final. In case you haven't heard of it yet: The compiler enforces, that a final variable is only assigned once. This allows me to make an Instance of Node single-use only. If you want to change an item, you will have to replace it with a whole new node.

Beware this does not prevent changes to Objects themselves. It is very possible to do:

private final Map<String, String> demonstration = new HashMap<String, String>();
public void doSomething() {
 demonstration.put("Demo", "Value");
}

The other thing is that I initialize next and previous not in the constructor. This is just my personal preference!

Additionally the inner class is static. This prevents access from the inner class to the outer one when using this. For more information have a look at this CR-Answer.

While we're at constructors / initialization:

##Consistency:

One big issue I see with your code, is that you don't work consistently:

public Node() {
 this.setNext(null);
 this.setPrev(null);
}

public List() {
 head = null;
}

You use quite the mix here. In the Node constructor you use the setters - even with this keyword - to instantiate your Node, in your list you do it directly - and without this - on the field.

While both may be legal, I personally prefer using the middle way. As you have seen in the code above I usually initialize fields by using the this, but not the setter methods. In case of equal names it's required to use this so I usually put it everywhere.


Example no. 2:

node<T> f = head, b = head.getPrev();

node<T> b = head.getPrev();
b.getPrev().setNext(head);

here you have some statements on one and the same line, while throughout the rest of your code, you mostly place statements on separate lines.

Here I advise to wherever possible use the second approach. One Statement has one line and one line has one (or no) statement.

Vogel612
  • 25.5k
  • 7
  • 59
  • 141
default

AltStyle によって変換されたページ (->オリジナル) /