Review for Release

[Werner Keil <we...@gmail.com>]

Eberhard,

It seems you have a slightly different structure in the C# client, but
mimick e.g. this class
http://svn.apache.org/viewvc/incubator/devicemap/trunk/devicemap/csharp/DeviceMap/parser/XmlParser.cs?view=log

Can you confirm if it is only used by a private method call in your port?


Coming from a C/procedual background I would not expect Reza to be working
in a Java or Object-oriented long enough to understand why a package for a
single class that is just a hidden implementation detail of a private class
is a bad thing and should be avoided.

A public (API) method in Loader
public Map<String, Device> getData() throws IOException {
internally calls the private

 /*
101     * loads device data from an *InputStreamReader*
102     */
103    private void loadDeviceData(Reader in) throws IOException {
104        XMLParser parser = new XMLParser(in);

So Device is part of the public API that should be exposed, but the simple
wrapper and local helper around Reader called XMLParser is an internal
implementation detail that is of no interest and value to others. Unless
it's passed as argument or returned by a public method in similar ways as
Device and a few other value types do, it must not be publicly exposed. The
next loader probably reads a DB or does something else, no common pattern
or (what I'd expect in a separate "parser" package) an abstract base class
Parser exists.

The intent of "some day" putting more does not justify a separate package
now. If a future release does, then let's do it at that point. An internal
helper or wrapper class (merely to save lines of code inside DDRLoader)
does not have to be public in proper OO and it saves us and users the
trouble of having to write JUnit tests[?] An argument to a public method
could be good for mock/test code, but that is not the case. A unit test
will test this from the outside by passing test data, such as yours, not a
mocked version of this internal helper class.

Is there something in the .NET code that does this completly different?

As soon as it seems in shape, we should try this Review Board (assuming it
is by other members, otherwise in a relatively small team with 1-3 active
people at times that's what happens here internally[?])

Thanks,


 Werner