mzahor / morelinq

Automatically exported from code.google.com/p/morelinq
Apache License 2.0
0 stars 0 forks source link

Code review request for reorg #23

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Purpose of code changes on this branch:

Re-organize MoreLINQ so that all operators are implemented under the
Enumerable class, providing several advantages over the current structure
that classifies operators under their so-called category class.

When reviewing my code changes, please focus on:

The branch puts all operators under a single class called Enumerable, much
like the base LINQ operators it extends. However, each operator resides in
its own file and uses a partial modifier to allow the Enumerable definition
to fan out. The benefits of this new structure over the current are the
following:

* Regions are not needed to superficially organize and delimit operators
within a class just because the class seems to be growing big as new
overloads and operators are added to it. Regions work fine as a visual
navigation aid if you are in a sophisticated IDE like VS, but not when you
are browsing the source through other means, such as the online Subversion
browser, terminal or e-mail.

* Categorization of the operator is more of a documentation and learning
aid than an implementation concern. Once the MoreLinq.Pull namespace is
imported, all types extending IEnumerable<T> are available irrespective of
how the authors categorized them. The point is, it doesn't serve as aid for
the user of the pull operators. In the new organization, categorization
could be done (if absolutely needed) with a custom XML doc summary element.

* A single partial Enumerable is simple than multiple category classes.
There is no added benefit of the latter. Naming LINQ operators is hard
enough. Proper categorization adds additional subjectivity and headache for
authors. Before you can add an operator, you need to think about how to
name it and categorize (the exercise may not be entirely unhealthy or
useless). With the new organization, you just put it under Enumerable.

* If an operator needs to be located, one has to first try and remember the
class category, open that file and then add the operator or overload an
existing at the right place. With the new organization, you just create a
new file, define Enumerable as partial and add the operator implementation.
For an existing operator, locating its source code is dead easy because the
file is named after the operator.

* Since each operator lies in it own file in the proposed new organization,
there is less contention or risk of edit conflicts between authors working
on the same source file.

* With the proposed new organization, providing on-line links to a single
operator and all its overloads is as simple as providing a URL the
operator's file, as in http://tinyurl.com/MoreLinq-DistinctBy, whereas with
the current organization, the best you can do is point someone to the
category class and then ask them to look for the operator under there.

The are, however, two downsides, none of which seem to outweigh the
aforementioned advantages:

* All 134 tests now appear under a single fat fixture. For now, I have
classified the operators by category using the Category attribute from
NUnit. With this, you can still decide to run tests of only a single
category (using, for example, the /include switch from nunit-console) or
visually distinguish them in the GUI.

* If someone wants to use an operator that is not implemented as an
extension method or invoke it without the extension invocation syntax, then
the compiler may be confused between System.Linq.Enumerable and
MoreLinq.Pull.Enumerable. The conflict occurs only if System.Linq and
MoreLinq.Pull are imported in the same source file. This could be resolved
in one of two ways: (i) we use MoreEnumerable instead of Enumerable or (ii)
let the user of the code decide by introducing type aliases to her liking,
as in:

using LinqEnumerable = System.Linq.Enumerable;
using MoreEnumerable = MoreLinq.Pull.Enumerable;

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by azizatif on 4 Apr 2009 at 10:27

GoogleCodeExporter commented 9 years ago

Original comment by azizatif on 4 Apr 2009 at 10:34

GoogleCodeExporter commented 9 years ago
See also:
http://groups.google.com/group/morelinq-dev/t/aee9babcbe997d73

Original comment by azizatif on 6 Apr 2009 at 1:48

GoogleCodeExporter commented 9 years ago
Merged in r82.

Original comment by azizatif on 6 Apr 2009 at 1:49

GoogleCodeExporter commented 9 years ago
Okay, it's all looking good, and I've made a few other changes - MoreLinq.Pull 
is now 
just MoreLinq, and Enumerable is now MoreEnumerable.

Original comment by jonathan.skeet on 6 Apr 2009 at 4:56