locked
Best practice regarding interface implementation? RRS feed

  • Question

  • I have had cause to declare a good number of interfaces recently and have found mnyself having to think creatively about two specific issues. As an example consider an ITranslatable interface.

    interface ITranslatable
    {
        // methods
        string GetCulturalName(string locale = "");
    
        // properties
        string InternalName { get; }
        string Locale { get; set; }
        Guid TranslationKey { get; set; }
    }

    Now, this could be applied to many classes so I don't want to provide a base class implementation since many of the implementors will already be derived from something more important. Instead I use code snippets to insert a skeleton implementation (within its own #region) and flesh that out. And that brings me to my two issues and me wondering if there is a better solution out there I just don't know about?

    1. Field declartions. Is it a bad idea / reasonable / a good idea to put field declarations for fields that exist specifically to support the interface implementation  within the #region where the interface is implemented? Or should all field declarations be molved to the top of the class so they are found where people conventionally expect them to be? Obviously I can't put them in the interface itself where some might feel they belong!:-)

    2. Static support methods. For example all implementors of the above ITranslatable MUST also provide a public static Dictionary<Guid,string> GetVocabulary() which returns all the internal strings for the class (ie for all possible instances) with their translation key (so a centralized Vocabulary Manager can build a full dictionary by simple polling all the Types that implement ITranslatable and Invoking GetVocabulary(). I can't put a static method declaration in the interface (a shame since the existence of GetVocabulary is very much part of the "contract" but invoking it should be at the class level). So at the moment I put the static declaration as a stub function into a code snippet with my nominal implementation. Is there a better way to establish this "contract"?

    Note: I experimented with an IVocabularyProvider interface but it achieves nothing directly because either GetVocabulary() must be static OR I have to build a collection class for the base entity JUST to implement a vocabulary lookup (and GetVocabulary would be an instance method on that). This is ridiculous for those classes that otherwise do not require collections - it would impose an expensive and unnecessary burden on implementors of such a contract.

    How do others deal with these issues? Is there a best practice or does everyone go their own way?

    Thanks in advance.

    Alistair


    Alistair

    Thursday, May 16, 2013 6:55 AM

Answers

  • Just responding to the first post (the second post was tl;dr and I don't understand what GetHashCode has to do with the original post)

    1) What fields?  I avoid defining my own backing fields if at all possible (hooray for automatic properties).  If you must have a backing field because your property implementation does something funky then you absolutely should have it declared immediately before the property.  The only code using the field should be the property so why force people to go searching through the code file?  Have you read "Code Complete" by Steve McConnell?  If not, get it and read it.  This sort of layout is discussed in Ch31 under "Laying Out Data Declarations" with the guiding principles laid out in Ch 10 under 'Keep Variables "Live" for as Short a Time as Possible'.

    You might also want to consider http://www.codeproject.com/Articles/392516/Why-I-use-explicit-interface-implementation-as-a-d Once I started following this advice my life got 7% better.

    If implementations of your interface are similar enough to have a code snippet do the job then I would consider creating a single implementation and creating an instance of that implementation in the classes that need it.  Most of the interface implementation would then just be wrappers for the single implementation.  If you need to change the implementation then you only do it once rather than having to track down all the places where the snippet was used.

    2) Interfaces (and abstract classes) can't have static members.  There are many discussions as to why this is the case that Google will reveal for you.  Bottom line, however much you think you want it there is no such thing as a static member of an interface.


    Paul Linton

    • Marked as answer by agillanders Friday, May 17, 2013 11:45 PM
    Friday, May 17, 2013 7:43 AM

All replies

  •  I have been trying to put together a checklist for my team that .NET developers should review before and after they design and code a new class to ensure that they are following good common conventions and practices.  I am aiming for brevity but also for completeness in terms of the most important and generally universal practices that will apply to most type definitions.  Some items were included because I often see them neglected, and some were omitted because I am assuming a certain level of developer knowledge and experience.  Most teams have "best practices" conventions for process, for design, for coding and coding convention, etc.  This is a “best practices” list for type implementation; it is intentionally incomplete but aims to cut at the core of what is most critical or most often overlooked.  Most of these will apply to structs as well as classes, but not so much interfaces and enumerations. 

        Assuming you have designed your API according to your needs and you sit down to start implementing types, the following list serves as a good guideline for following some best practices that apply to almost all type definitions.  Some of these are obvious to seasoned developers and some I often see overlooked that would have been a strong return on investment representing great utility for minimum expenditure of effort.  These are not set in stone but I recommend that every time you write a class you should go through this checklist and make sure that you have employed each of these common recommendations, barring exceptional scenarios where you have a strong reason for exclusion that you can actually articulate (this does not include “I didn’t have time”).  This may still require some tweaking, so any feedback would be welcome.

     

    So, without further ado, 10 things you should do to every C# class that you write:

    1. Define an interface for every class that you write, even if it doesn't make much sense to implement and compile the interface.

    2. Use properties to encapsulate all public (and private) data points.

    3. Make every class Serializable

    4. Provide a default constructor if possible

    5. Always override System.Object.ToString()

    6. Avoid overriding System.Object.Equals(), System.Object.GetHashCode(), and avoid implementing ICloneable. 

    7. Examine your class and the most common .NET interfaces and determine if it makes sense to implement any of them, particularly the "system aware" interfaces that are used frequently such as IComparable<T> / IComparer<T>, IEnumerable<T> / IEnumerator<T>, and IDisposable.  Prefer IComparer to IComparable.

    8. Use XML comments to document all members.

    9. Think about your namespace, don't re-invent the wheel, and don’t leave any “broken windows”.

    10. Replace all System.Collections members with System.Collections.Generics counter-parts.


    __________

    1. Define an interface for every class that you write, even if it doesn't make much sense to implement and compile the interface. 

         This is always a good place to start when you set out to start defining and implementing a class.  Doing so helps you with the design process and helps you to think through your public interface / API.  When you start defining a class you should generally begin by first hashing out a rough draft interface specification.  This helps you to think about the purpose of the class and how its functionality will be exposed and utilized.  The requirement for concrete interfaces that you will actually compile will usually have already been identified when you designed your type hierarchy / API, but typing up an interface before you start implementing a class is almost always beneficial and sometimes leads to eureka moment design improvements.  It also helps give you direction as you start to implement.  It may or may not be useful to actually compile and implement the interface.

     

    2. Use properties to encapsulate all public (and private) data points. 

        None of your classes should include any publically accessible fields in their definitions.  This pattern is ubiquitous across the .NET Framework.  All fields should be private or protected and should be wrapped in a property providing get and set accessors and mutators.  It is a good practice to code so that even your private fields are accessed within the class definition through properties rather than directly.  As far as your public interface goes, properties provide encapsulation, can be used for data-binding whereas fields cannot, as well as provides your consumers with a consistent interface.  Even if you do not require any accessor or mutator logic when you first write your class wrapping your fields within properties will allow you to add accessor and mutator logic later on down the road if it becomes necessary without breaking any consumer code that was compiled against your class definition.  If you do not require accessor and mutator logic C# provides a very time saving and handy dandy construct by allowing you to implement automatic properties, as opposed to the days of yore when encapsulating fields meant declaring a bunch of variables and then declaring corresponding accessor and mutator methods.  There is no good reason not to always employ properties to control access to your field members and preserve the integrity of your state data. 

         A side note, I prefer to utilize a consistent naming convention for field identifiers in order to help differentiate them from properties.  Many developers will prefix their field names with an underscore as in _myField, and I find it useful to follow this convention.

     

    3. Make every class Serializable 

        I won’t go in depth into serialization or into related Windows Communication Foundation (WCF) technologies, but suffice to say, making your classes serializable requires very little code and can allow for versatile capability when it comes to persistence and transport.  By doing so your class state can be persisted across instances of the application or easily ported for transport via remoting or web services should the need arise.

        Considering the low cost of memory, storage, and CPU cycles in today's mammoth PCs and servers, I generally recommend that you prefer Simple Object Access Protocol (SOAP) formatting for serialization rather than binary.  Binary serialization is certainly the most compact and efficient, but SOAP serialization is more versatile and more portable in general.  If down the road you realize that you want to be able to transport your serialized classes across HTTPS via web services, for example, you will be glad that your type already supports SOAP formatted serialization.  If, however, you have some specific circumstance or reason why you need to serialize objects in the most storage space efficient manner possible, then utilize a binary formatter to serialize your type.

        If you know from the outset that your class will require serialization to be consumed by non-CLR code (often via web services), it is better to use serialization provided by the XML namespace rather than SOAP serialization.  Obviously binary serialization will not be applicable.  In general, however, I prefer the SOAP formatter for general purpose serialization as XML serialization can get more complicated because only public members are serialized and it is not cost-beneficial to define the implementation unless you have a specific requirement to do so.  On the other hand, SOAP serialization functionality can be added to each and every class with only a few lines of code.  Doing so adds a lot of useful functionality and can pay back great dividends down the road.  If you do not mark your classes as serializable then other classes that use your type cannot support serialization either, so it is best to support this behavior from the outset in all of your classes.   

     

    4. Provide a default constructor if possible 

        Recall that the C# compiler automatically generates a default no parameter constructor on your behalf in the event that you omit any constructors.  If you supply a custom constructor then the default constructor is no longer automatically generated behind the scenes, and you will need to implement it explicitly.  A lot of code that leverages your type later will be easier to write if it can assume a default constructor for your type (hence, one of the generics constraints available in C# is the condition that the type parameter be a type that provides a default constructor).  Note, however, that there will be times when you do not want to provide a default constructor for very specific reasons, but these are relatively uncommon.

        As far as constructors are concerned, there are a few other rules of thumb.  Do your best to avoid duplicating initialization logic by utilizing constructor chaining or optional parameters.  Also, in almost all cases you are going to want to initialize static members in a static constructor, not in your instance constructors.  Additionally, you should generally prefer to initialize variables with values when you declare them rather than assign them in your constructors.

        More on constructors: CodeHustle - C# Constructors

     

    5. Always override System.Object.ToString() 

        Overriding System.Object.ToString() allows you to represent your type or your type’s state in a human readable format.  This is useful for displaying information to your users, binding to GUI controls, and also for debugging.  This is one of the most commonly used methods in the .NET environment, and all of your types should provide their own specific implementation. The default behavior for System.Object.ToString() is to return a textual representation of the type name, which is not very useful.  You should override this method to provide the simplest, most natural, most reasonable string representation for your class which will reflect the most commonly used representation.  For simpler data structures this may entail returning a single textual value (i.e. employee name, string conversion of numerical data points, etc.).  For more complex types you will want to return a string representation of the internal state of each important property or field (this is particularly useful for debugging).

     

    Blog1

         If you want to provide multiple implementations of ToString() that are useful for different scenarios, such as returning a single string version of a key identifier (i.e. employee name) for UI output, and also a detailed string representation of internal state for debugging purposes, a common practice is to use IFormattable, IFormatProvider, or ICustomFormatter which will allow your ToString() implementation to accept formatting parameters and return the ToString() result that you require.  This can be useful in the event that you want different string representations of your type available for different user output scenarios (i.e. on version for textbox output, one version for HTML output, etc.).

    IFormattable

    IFormatProvider

    ICustomFormatter

     

        Overriding ToString() is particularly important for structs in order to avoid boxing and unboxing penalties when ToString() is called.  All structs and classes that you define will benefit from overriding ToString() in order to return a textual representation of the type’s current state.

     

    6. Avoid overriding System.Object.Equals(), System.Object.GetHashCode(), and avoid implementing ICloneable. 

                Many programmers will commonly override System.Object.Equals() in reference types in order to be able to utilize value-based semantics for comparison between two instances instead of the default behavior of comparing references.  This should be avoided.  Be aware that if you override System.Object.Equals() you must also override System.Object.GetHashCode() or you will break any Hashtable implementations that are used as a container for instances of your type because two objects for which Equals() returns true should always have the same hash code.  Often programmers will override both GetHashCode() and Equals() leveraging their ToString() implementation.

    Blog2

         I am going to recommend that, as a best practice, YOU DO NOT OVERRIDE either Equals() or GetHashCode().  Unfortunately, I have read several C# and VB.NET books that recommend this practice, but as it turns out, there is a very large danger in doing this.  You risk breaking any Hashtable collections that might be used to store your types if you do this improperly.  You cannot use mutable types to determine the results of Equals() or GetHashCode() methods if you want Hashtables to work properly with your type, but at the same time, if you do exclude any mutable members in your calculation of Equals() then you will potentially get incorrect results when attempting to perform value-based comparisons between separate instances of your reference type.  If you follow the pattern above it does not matter that strings are immutable, because you are still risking either using or excluding mutable types in the calculation of the string value.

        Hashtables work by using an immutable invariant property of your type that represents it uniquely.  This value is used as input to a hash function which returns a hash code that determines what “bucket” in the hash table data structure to place the item.  When items are retrieved from the hash table the hash code is calculated for the item you are searching for in order to determine what bucket to look for it in, and the Equals() function is used to determine which specific item in the bucket is the correct one to retrieve.  This makes Hashtables ideal over other data structures for fast item retrieval if the collection is very large.  If the data that is used to calculate the hash code is mutable and has been changed since it was stored in the hash table, the hash table might end up looking for the item in the wrong bucket.  On the other hand, if you are trying to use value based semantics instead of comparing references and do not include any mutable members that your type contains in your calculation of Equals(), you might end up with incorrect results.  This is why the default implementation of Equals() and GetHashCode() are based upon an internal object identify field generated when an object is placed on the stack, or in the case of a struct, the first field you define (the first field you define in a struct should ALWAYS be an immutable invariant whether or not you are overriding Equals() and GetHashCode()).

        The best scenario for safely overriding Equals() and GetHashCode() is for immutable value types.  It is best to ensure that the field/s you utilize for Equals() and GetHashCode() are unique among multiple instances of your type, have an even distribution among the range of possible values, are invariant, and must be immutable.  For more see Bertrand Leroy’s blog post:

    When (not) to override Equals?

     

         Another thing to avoid if possible is implementing ICloneable.  ICloneable is most often used to provide a full deep copy of your object instance (rather than a shallow copy which just copies references and not actual values).  Realize that to support the generation of cloned instances a class can contain only member variables that are either value types or that are reference types which also implement ICloneable.  Also realize that this burden is going to extend to any classes that derive from your class in the future as they will be required to support the interface as well.  This can get quite messy and require a lot of ICloneable implementations.

          In summary, always override ToString () for both structs and classes, and avoid overriding Equals() and GetHashCode().  If you want to be able to perform comparisons between reference instances using value based semantics it is best to define another method apart from the System.Object.Equals() override.  You can take a short-cut in this method and leverage your ToString () result if you provided a complete ToString () implementation that includes all key fields and properties in your type and up the type’s inheritance chain.  Always, always, always override ToString ().  Avoid implementing ICloneable if possible, and avoid overriding Equals() and GetHashCode(), but at a minimum understand the ramifications if you do.  Even if you do not plan to use your type in a Hashtable collection you cannot make the assumption that other developers will not need to in the future.

     

    7. Examine your class and the most common .NET interfaces and determine if it makes sense to implement any of them, particularly the "system aware" interfaces that are used frequently such as IComparable<T> / IComparer<T>, IEnumerable<T> / IEnumerator<T>, and IDisposable.  Prefer IComparer to IComparable. 

         There are standard as well as generic counterparts to most of these interfaces. The generic ones should be preferred to the non-generic versions.  A sizeable percentage of all classes that you write will benefit from implementing one or more of these interfaces.  If it makes sense at all to implement any of them, go ahead and do so.  Doing this keeps in step with the common practices employed throughout the framework and may pay off large dividends down the line.  If your type is ever going to be stored within a collection (likely), then IComparable / IComparer should be implemented to define sorting results.  If your type is a collection then IEnumerable / IEnumerator should be supported to enable for each loops.  If your type encapsulates any unmanaged resources you will obviously want to implement IDisposable.

         IComparable and IComparer deserve special mention here.  Normally IComparable is used when your type has a natural intrinsic comparison construct and IComparer when you want to implement alternative comparison rules providing multiple specialized ways to sort your items other than just the standard natural ordering relationship.  IComparable is implemented and defined by your type and specifies a CompareTo() method which accepts one other object instance as a parameter, while IComparer is used to accept two instances of your type and is often implemented in a helper class that you define.  A common practice is to use containment / delegation to hold an instance of the helper class implementing the interface.  Sorting methods in collections objects will accept a reference to your IComparer as a parameter when called and will use it to determine comparison results when sorting your type.

         Those are the common usage patterns; however, there is another critical consideration that leads me to recommend you to tend towards utilizing IComparable only for value types / structs, if at all, and always using IComparer for defining sorting rules for reference types.  Microsoft recommends that you override Equals(), and the equality, inequality, greater than, and less than operators if you utilize IComparable.

    See Microsoft Code Analysis warning CA1036

    http://msdn.microsoft.com/en-us/library/ms182163.aspx

     

         Since we have already covered the inherent dangers of overriding Equals() with reference types, I recommend that you use IComparer with reference types in order to eliminate this issue.  IComparable is fine for use with immutable types if you follow the rules as to a proper override of Equals() in a struct (requiring a correct override of GetHashCode() and the equality operators).  Personally I would just recommend that you prefer IComparer to IComparable.

                A final note on IDisposable, if you implement this interface on a type that contains unmanaged resources you need to also implement a finalizer to ensure that those resources are properly released by the garbage collector in the event that the client code does not call your Dispose() method.  There are some common patterns to implementing IDisposable() that I will go into more detail in another post.  It would behoove you to do this properly.

     

    8. Use XML comments to document all members. 

        There is absolutely no reason at all not to utilize the .NET XML comments functionality.  Doing so not only allows you to generate documentation on your types, but it allows you and your coworkers to have intellisense information available when writing code that consumes your types.

     

    9. Think about your namespace, don't re-invent the wheel, and don’t leave any “broken windows”. 

         Common practice is that all of an organization’s code goes under a top-level namespace that is named after their organization.  Namespace organization after that should be dictated by project or functionality of your types.  I see a lot of organizations practice code-reuse on the class level, but rather than reusing assemblies and namespaces across the enterprise developers have a tendency to start writing a class, remember that they wrote a class with similar functionality last year, and then go copy and paste the class into a new project.  This is bad.  It is bad from a maintainability stand-point and it is bad for the rest of the developers in the organization.  If you have a library of types that map to a specific area of functionality or represent the entities in a specific database etc., reuse the existing shared assembly rather than re-inventing the wheel (and creating a maintenance nightmare) if at all possible.  Doing this not only saves you time, but organizing your types appropriately helps the team by allowing other developers to find and leverage existing functionality.

         Related to this is finding opportunities to refactor code into a base class.  If you wrote a class that might share a lot of actual implementation functionality with another class in your API but your initial design or specification did not call for a common base class between the two, go ahead and move that common functionality to a common base class.

         These things might seem a little software engineering 101, but we are all guilty of trying to cut corners when under the gun, and it will come back to bite you in the ass later.  Andrew Hunt and David Thomas, authors of “The Pragmatic Programmer” refer to this as leaving “broken windows”. 


     

    10. Replace all System.Collections members with System.Collections.Generics counter-parts.

        I include this mainly because this should be an absolute given from the get-go, but for some reason people are still using System.Collections extensively.  Please stop.


    Stop using System.Collections!!!


    Please mark the post as answer if it is helpfull to you


    Thursday, May 16, 2013 9:33 AM
  • Wow. Way to hijack a question for advancing a personal perspective! You did not address either of the two issues I was asking about but instead gave me your opinion on a lot of stuf I did not ask about (and actually don't agree with).

    Your 10 points are interesting and many have value but one aspect (point 6) has an alternative solution that then cascades into much of the rest. In short I completely disagree with point 6; or rather I agree with the concern but completely disagree with the conclusion.

    I agree that if you override GetHashCode() then you need to learn how to do it properly. But it is pretty straightforwad after that and you can stop being afraid of it. Having value based comparisons/equality for reference types is EXTREMELY useful and MUCH more intuitive in use than referential equality. IComparable, Equality and GetHashCode can be used safely and effectively to achieve that...mutable type or not.

    At the vey least you can declare an anonymous type using exactly the same value fields as you use for equality etc and call GetHasCode() on that (or value properties of any reference fields). The default implementation of that uses prime numbers as offset and weightings to combine the hashcodes to produce an effective well behaved result. Personally I implement a version of that directly to eliminate the overhead of a method call and you may want to go further if your type will be used in a very large hash table. Otherwise this default is a good solution.

    Also, if I feel the need to derive my own hash I use a recognized algorithm (see JWalkers review for a good foundation of the issues). Peronally I use a version of FNV-1a for any direct hash implementations I need...but I have not yet had need for hashing on a trully grand scale that might justify more sophisticated solutions.

    My snippet for GetHashCode is:

    public override int GetHashCode() 
    { // Remember: Two objects considered equal MUST have the same hash code
    
        /// IMPLEMENT: CalculateHashCode() still needs to be implemented.
        throw new NotImplementedException("The CalculateHashCode() method has not yet been implemented.");
                
        int hash = 17;
        unchecked
        { //for each field.GetHashCode() in the key        
            hash = hash * 23 + Field1.GetHashCode();
            hash = hash * 23 + Field2.GetHashCode();
            hash = hash * 23 + Field3.GetHashCode();
        }
        return hash;
    }
    

    Alistair


    Alistair

    Thursday, May 16, 2013 5:18 PM
  • Well Alistair

    I will check that.


    Please mark the post as answer if it is helpfull to you

    Friday, May 17, 2013 6:57 AM
  • Just responding to the first post (the second post was tl;dr and I don't understand what GetHashCode has to do with the original post)

    1) What fields?  I avoid defining my own backing fields if at all possible (hooray for automatic properties).  If you must have a backing field because your property implementation does something funky then you absolutely should have it declared immediately before the property.  The only code using the field should be the property so why force people to go searching through the code file?  Have you read "Code Complete" by Steve McConnell?  If not, get it and read it.  This sort of layout is discussed in Ch31 under "Laying Out Data Declarations" with the guiding principles laid out in Ch 10 under 'Keep Variables "Live" for as Short a Time as Possible'.

    You might also want to consider http://www.codeproject.com/Articles/392516/Why-I-use-explicit-interface-implementation-as-a-d Once I started following this advice my life got 7% better.

    If implementations of your interface are similar enough to have a code snippet do the job then I would consider creating a single implementation and creating an instance of that implementation in the classes that need it.  Most of the interface implementation would then just be wrappers for the single implementation.  If you need to change the implementation then you only do it once rather than having to track down all the places where the snippet was used.

    2) Interfaces (and abstract classes) can't have static members.  There are many discussions as to why this is the case that Google will reveal for you.  Bottom line, however much you think you want it there is no such thing as a static member of an interface.


    Paul Linton

    • Marked as answer by agillanders Friday, May 17, 2013 11:45 PM
    Friday, May 17, 2013 7:43 AM
  • Hi Paul,

    Quite right...the hashcode stuff was in response to Amersh. His reply was not on point but had some unrelated merit.

    Thanks for your considered response too. It is some time since I read Code Complete - I'll go dig it out again it could stand another read.

    I like your suggestion of wrapping the interface implementation into it's own class to be instantiated by the implementor (cf a base class to be inherited). That is thought provoking but could be an excellent path forward for improving maintainability albeint it adds another layer of indirection to users of the class.

    Regarding backing fields. I too love the auto implemented properties however that is all they are. The compiler simply provides backing fields for you. Given that, I have no issue with being more explicit. And yes, it is mostly done when I am:

    • initializing the field before the constructor (I generally prefer to initialize in the constructor so initialization code is localized but if I have multiple constructors and the initialization is always the same...). 
    • doing some validation in the set procedure - very common - I like to prevent 'garbage in' whenever possible.
    • it is going to be passed as a ref or out parameter to external functions (a property has no memory location after all - it is a method call).

    The thing is that I do all of that quite a lot - if that is funky then so be it. I used to be concerned about a method call vs a memory fetch but since the compiler inlines the methods when it can that concern has subsided.

    Elsewhere I got a really simple suggestion that is so obvious I missed it. I still declare most fields in a declaration block at the top of the class definition (I want to know at a glance what non-local variablesI am committing the class to and scattering field declarations around the code is not doing it for me). However I simply add a // ITranslatable fields comment above them. Now I can see all fields in one place, and what they serve. Duh!

    Your suggestion of an implementation as a wrapper class is a great one although it doesn't solve the static method issue - I still need snippets for that until they give us static methods in an interface (anyone holding their breath?) You definitely get a partial for that!:-)


    Alistair


    Friday, May 17, 2013 11:41 PM