[Code Review] – Reinventing the Wheel

This is a code change that I observe today. It has been in the codebase for a long time, and I find it interesting to talk about it.

re-inventing-the-wheel-coding-review-linq

The LINQ is out since .NET 3.5, and all above code is just doing one thing: select the specific types e.g. either LayoutAnt or LayoutDevice from a member list layoutList. And one line of LINQ does this exactly: OfType<>

The left version is OK, but it is just old-school considering the developer who did not get familiar with LINQ. But the right version is worse. It has added a ref List which will be cleared. This is not unit-testing friendly at all.

The private methods are not unit-test friendly. And one big issue for both methods is: the member variable layoutList is referenced. If you really have to re-invent the wheel, at least make it a public, static method which takes a readonly layoutList and returns a new copy of List. In this case, the entire function is immutable, and unit test friendly.

You may also like: 代码审核之 重新造轮子

–EOF (The Ultimate Computing & Technology Blog) —