Simple code review

Jan 14, 2012 at 8:28 PM

Much of these are probably decisions you made for how you want your code to look - so grain of salt:

A more general comment (with a few details below), but you might consider making your circular list allow any integer as an index and have it loop around. So any action that takes an index would work the following way:

private _interalArray = [ "1", "2", "3", "4", null, null]

then

this[3] = "4"; //this works today

this[6] = "3"; //this would throw an error today, but instead I suggest maybe it loops around so when it gets to "4" it then loops to the beginning

You have a lot of place where it will throw exceptions if the index isn't in range

 

Line 29, many of these base interfaces are redundant.

public class CircularList<T> : IList<T>, ICollection<T>, IEnumerable<T>, IList, ICollection, IEnumerable, IDisposable
ICollection<T>, IEnumerable<T>, ICollection, IEnumerable are all redunant

Line 44: private int capacity; can be made read only

Line 54: private int tail; is never used

Line 59: private ReaderWriterLockSlim locker = new ReaderWriterLockSlim(); can be made read only

Line 76: public int Count can actually be converted to an auto property with a private setter

Line 155: public this[int index]; you throw an exeception in the setter and I understand why. However, I think it's perfectly valid to set the node of a list to a new value - this shouldn't thow an error. ie (where 0 = null / not initialized)

1 -> 2 -> 3 -> 0 -> 0 -> 0

Here, changing 2 to say 4 should be perfectly valid, but setting 0 to 4 shouldn't make sense. You should be able to check if the current place has a value or not and allow the change or toss an error. Alternatively, you could wrap around, so that in this example if they call this[5] = "wer", then you would have this[0] = 1, this[1] = 2, this[2] = 3, this[3] = 1, this[4] = 2, this[5] = 3, now set that to "wer". This kind of helps reinforce that it's circular (maybe)

Line 212: public void Insert(int index, T item) has a ton of unreachable code becuase of the thrown exception in the first line. However, similar to this[index], I don't see why they shouldn't be able to insert an item into the middle of the middle of a circular list. If you do implement it, make sure it works exactly as this[index] (circular counting or throws an index out of range error

Line 342: public bool Remove(T item)  bool remove could be turned into a constant, it looks like this method always returns false, even though the message says could return true. If it always returns false, then you could remove the remove declaration and simply have return false at the bottom

Line 448: public void Insert(int index, object value), unreachable code in this method

There are a ton of "this." and places where you could use "var" instead of "T" without losing readability - but those are very much with coding conventions you choose to use.

Coordinator
Mar 25, 2012 at 11:24 AM
  1. "you might consider making your circular list allow any integer as an index and have it loop around."
    • Acknowledged: No Action :- This functionality can easly be achived from the calling code, and would casue un unessersy overhead if performed each time the index was used. For now I'm happy with the default list like behaviour e.g. put two items in index the third you get an index out of bounds exception.
  2. "many of these base interfaces are redundant"
    • Fixed: Unessersery declaration or interface implementation removed
  3. "private int capacity; can be made read only"
    • Fixed: now read only.
  4. "private int tail; is never used"
    • Fixed: Tail removed.
  5. "private ReaderWriterLockSlim locker = new ReaderWriterLockSlim(); can be made read only"
    • Fixed: locker now readonly
  6. "public int Count can actually be converted to an auto property with a private setter"
    • Fixed: Count Property is now auto with private set
  7. "public this[int index]; you throw an exeception in the setter"
    • Acknowledged: No Action :- The order of the items in the collection can only controlled by the collection, this is the intended functionality. Besided what you have suggested would make the list circular only over the items already in the list not up to the capacity set in the constructor.
  8. "public void Insert(int index, T item) has a ton of unreachable code becuase of the thrown exception in the first line"
    • Fixed: Unreachable code removed. Method has been marked as Obsolete
  9. "public bool Remove(T item)   looks like this method always returns false
    • Fixed: The return value is now dependant on the item passed in being in the collection
  10. "public void Insert(int index, object value), unreachable code in this method"
    • Fixed: Unreachable code removed. Method has been marked as Obsolete
  11. "here are a ton of "this." and places where you could use "var" instead of "T" without losing readability - but those are very much with coding conventions you choose to use."
    • Acknowledged: No Action :-
      The use of "this" in front of references to member variables and methods is a stylecop rule i happen to agree with.
      I beleave that var should only be used for actual anoymus type, using it where a type is available is nearly always just lazieness a relies on you remebering the type (in six months it may be less obvious)