At work we have (finally) started code reviews. Basically I got together with the other developers at my site and said 'lets do something', and well I think its going well so far.
Anyway, one issue we haven't covered yet is collection classes. I came across a class recently that initialised its collection to null, and the argument is that this leads to an easy test whether the collection is empty.
I think this is wrong, as it leads to mulitple states that equate to empty. Consider this...
Parent parent = new Parent();
Item x = new Item();
parent.Collection.Add(x);
parent.Collection.Remove(x);
The collection of the parent HAS to be initialised or we end up with a null reference in the get_Collection. What about when we have removed the object, how do we detect that the collection is now empty? It would basically require some form of link back from the collection to the parent to reinitialise? But thats not simple.
However when we integrate the collection into the parent then we have a different scenario, (I think this is even worse).
Parent parent = new Parent();
Item x = new Item();
parent.Add(x);
parent.Remove(x);
Now we can easily detect when the collection has a Count of 0 and therefore kill the collection. However look at what we have lost in order to get to this, before we could have performed
foreach(Item in parent.Collection)
{...}
Now we would have to implement a GetEnumerator() on the collection, but what happens if the parent has more that one collection, consider this, first with exposed collections
foreach(Item item in parent.ItemCollection)
{
parent.ReportCollection.Add(GenerateReport(Item));
}
foreach(Report report in parent.ReportCollection)
{
report.Print();
}
Then with an integrated collection.
foreach(Item item in parent)
{
parent.Add(GenerateReport(Item));
}
foreach(Report report in parent)
{
report.Print();
}
Theoretically we could attempt to implement multiple GetEnumerators() using Generics, but again this is not simple. We also have to have overloaded Add methods. At this point you would probably query that this code has been designed or evolved.
Personally I would argue that it is need of refactoring.
Subscribe