[Mono-dev] [PATCH]TraceSource stubs

[Jonathan Pryor]

On Tue, 2006-02-07 at 22:46 -0500, joel reed wrote:
> The attached patch adds 3 .Net 2.0 enums and stubs out the 
> methods and properties for the TraceSource class. I saw some
> stubbed out classes in the tree which used:
> 
>   throw new Exception ("The method or operation is not implemented.");
> 
> So i copied this approach till I have time to hack out the rest.

Please don't throw Exception.  Throw NotImplementedException instead.
 
> +#if NET_2_0
> +
> +namespace System.Diagnostics {
> +
> +       [FlagsAttribute]
> +       public enum SourceLevels {
> +               ActivityTracing= 0,
> +               All = 1,
> +               Critical = 2,
> +               Error = 4,
> +               Information = 8,
> +               Off = 16,
> +               Verbose = 32,
> +               Warning = 64
> +       } // SourceLevels
> +
> +} // System.Diagnostics

Generally you don't need comments indicating the end of a
class/namespace/enum/etc.  That's what "bouncing" is for in your editor
(e.g. '%' in vim will take you to the matching brace, paren, etc.).

> +namespace System.Diagnostics {
> +
> +       public class TraceSource {
> +
> +               public void Close()
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

As mentioned above, please use NotImplementedException.

> +               public bool Equals(Object o)
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

Equals would need to be marked `override`, but I seriously doubt that
you actually need to override `Equals'.  The MSDN docs should explicitly
state whether or not the method is overridden; if it isn't, don't
include the method.

In fact, MSDN2 states "(Inherited from Object.)", which means you don't
need to include this member.

> +               public bool Equals(Object x, Object y)
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

Like this one: it's provided by System.Object, and it's actually a
`static' member.  You don't need to include it.

> +               public void Flush()
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }
> +
> +               public virtual int GetHashCode()
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

You only need GetHashCode() if you override Equals(), and you wouldn't
mark it `virtual', you'd mark it `override' (if you needed it at all).

MSDN2 states that it's inherited from System.Object, so you don't need
to include this member.

> +               public Type GetType()
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

You *absolutely* don't need to provide GetType().

> +               public string ToString()
> +               {
> +                       throw new Exception ("The method or operation is not implemented.");
> +               }

MSDN states that this member is inherited, so don't include it.

> +               [ConditionalAttribute("TRACE")] 

Use [Conditional("TRACE")].  The "Attribute" is not needed.


Overall, the patch looks good.  One last remark, though:

Currently, it is preferred that unimplemented members _not_ exist.  This
makes it easier to determine if applications originally written
under .NET will work under Mono -- just try to compile it, and if it
compiles, it will _probably_ work.  (There are many exceptions, but this
is the ideal situation.)

Consequently, after fixing your code you should enclose all members
within `#if false` blocks (or just comment them out).  That way, the
source will exist (to help the next person who comes along -- less code
for them to write), but it won't cause people to be mislead when
compiling under Mono.  In the case of TraceSource, which is completely
un-implemented, you might consider just `#if false`ing the entire type.

Thanks,
 - Jon