Puneet Ghanshani

Chief Architect at Microsoft

12 minute read

No enterprise application exists without a method, event, delegate and every developer would have written methods in his/her application.  While defining these methods/delegates/events, we follow a standard definitions.  Apart from these definitions there are some best practices that one should follow to ensure that method does not leave any object references, other methods are called in appropriate way, arguments to parameters are validated and many more. 

This article outlines some of the anti-patterns while using Method, Delegate and Event and in-effect highlights the best practices to be followed to get the best performance of your application and have a very low memory footprint.

The right disposal of objects

We have seen multiple demonstrations that implementing IDisposable interface (in class BaseClass) and wrapping its object instance in ‘using’ block is sufficient to have a good clean-up process.  While this is true in most of the cases, this approach does not guarantee that derived classes (let’s say DerivedClass) will have the same clean-up behaviour as that of the base class.

To ensure that all derived classes take responsibility of cleaning up their resources, it is advisable to add an additional virtual method in the BaseClass that is overridden in the DerivedClass and cleanup is done appropriately.  One such implementation would look like,

  1. public class BaseClass : IDisposable
  2. {
  3.     protected virtual void Dispose(bool requiresDispose)
  4.     {
  5.         if (requiresDispose)
  6.         {
  7.             // dispose the objects
  8.         }
  9.     }
  10.  
  11.     public void Dispose()
  12.     {
  13.         Dispose(true);
  14.         GC.SuppressFinalize(this);
  15.     }
  16.  
  17.     ~BaseClass()
  18.     {
  19.         Dispose(false);
  20.     }
  21. }
  22.  
  23. public class DerivedClass: BaseClass
  24. {
  25.     // some members here    
  26.  
  27.     protected override void Dispose(bool requiresDispose)
  28.     {
  29.         // Dispose derived class members
  30.         base.Dispose(requiresDispose);
  31.     }
  32. }

This implementation assures that the object is not stuck in finalizer queue when the object is wrapped in ‘using’ block and members of both BaseClass and DerivedClass are freed from the memory

The return value of a method can cause a leak

While most of our focus is on freeing the resources used inside the method, it is the return value of the method that also occupies memory space.   If you are returning an object, the memory space occupied (but not used) is large

Let’s see some bad piece of code that can leave some unwanted objects in memory. 

  1. public void MethodWhoseReturnValueIsNotUsed(string input)
  2. {
  3.     if (!string.IsNullOrEmpty(input))
  4.     {
  5.         // value is not used any where
  6.         input.Replace(" ", "_");
  7.  
  8.         // another example
  9.         new MethodAntiPatterns();
  10.     }
  11. }

Most of the string methods like Replace, Trim (and its variants), Remove, IndexOf and alike return a ‘new’ string value instead of manipulating the ‘input’ string.  Even if the output of these methods is not used, CLR will create a variable and store it in memory.  Another similar example is creation of an object that is never used (ref: MethodAntiPattern object in the example)

Virtual methods in constructor can cause issues

The heading speaks for itself.  When calling virtual methods from constructor of ABaseClass, you can not guarantee that the ADerivedClass would have been instantiated.

  1. public partial class ABaseClass
  2. {
  3.     protected bool init = false;
  4.     public ABaseClass()
  5.     {
  6.         Console.WriteLine(".ctor - base");
  7.         DoWork();
  8.     }
  9.  
  10.     protected virtual void DoWork()
  11.     {
  12.         Console.WriteLine("dowork - base >> "
  13.             + init);
  14.     }
  15. }
  16.  
  17. public partial class ADerivedClass: ABaseClass
  18. {
  19.     public ADerivedClass()
  20.     {
  21.         Console.WriteLine(".ctor - derived");
  22.         init = true;
  23.     }
  24.  
  25.     protected override void DoWork()
  26.     {
  27.         Console.WriteLine("dowork - derived >> "
  28.             + init);
  29.             
  30.         base.DoWork();
  31.     }
  32. }

 

Use SecurityCritical attribute for code that requires elevated privileges

Accessing of critical code from a non-critical block is not a good practice.

Mark methods and delegates that require elevated privileges with SecurityCritical attribute and ensure that only the right (with elevated privileges) code can call those methods or delegates

  1. [SecurityCritical]
  2. public delegate void CriticalDelegate();
  3.  
  4. public class DelegateAntiPattern
  5. {
  6.     public void Experiment()
  7.     {
  8.         CriticalDelegate critical  = new CriticalDelegate(CriticalMethod);
  9.  
  10.         // Should not call a non-critical method or vice-versa
  11.         CriticalDelegate nonCritical = new CriticalDelegate(NonCriticalMethod);
  12.     }
  13.  
  14.     // Should not be called from non-critical delegate
  15.     [SecurityCritical]
  16.     private void CriticalMethod() {}
  17.         
  18.     private void NonCriticalMethod() { }
  19. }


 

Override GetHashCode when using overriding Equals method

When you are overriding the Equals method to do object comparisons, you would typically choose one or more (mandatory) fields to check if 2 objects are same.  So your Equal method would look like,

  1. public class User
  2. {
  3.     public string Name { get; set; }
  4.     public int Id { get; set; }
  5.  
  6.     //optional for comparison
  7.     public string PhoneNumber { get; set; }
  8.  
  9.     public override bool Equals(object obj)
  10.     {
  11.         if (obj == null) return false;
  12.  
  13.         var input = obj as User;
  14.         return input != null &&
  15.             (input.Name == Name && input.Id == Id);
  16.     }
  17. }

 

Now this approach checks if all mandatory field values are same.  This looks good in an example for demonstration, but when you are dealing with business entities this method becomes an anti-pattern.  The best approach for such comparisons would be to rely on GetHashCode to find out if the object references are the same

  1. public override bool Equals(object obj)
  2. {
  3.     if (obj == null) return false;
  4.  
  5.     var input = obj as User;
  6.     return input == this;
  7. }
  8.  
  9. public override int GetHashCode()
  10. {
  11.     unchecked
  12.     {
  13.         // 17 and 23 are combinations for XOR
  14.         // this algorithm is used in C# compiler
  15.         // for anonymous types
  16.         int hash = 17;
  17.         hash = hash * 23 + Name.GetHashCode();
  18.         hash = hash * 23 + Id.GetHashCode();
  19.         return hash;
  20.     }
  21. }

You can use any hashing algorithm here to compute a hash of an object.  In this case, comparisons happen between computed hash of objects (int values) which will be more accurate, faster and scalable when you are adding new properties for comparison.

Detach the events when not in use

Is it necessary to remove event handler explicitly in C#?  Yes if you are looking for lower memory footprint of your application.  Leaving the events subscribed is an anti-pattern.

Let’s understand the reason by an example

  1. public class Publisher
  2. {
  3.     public event EventHandler Completed;
  4.     public void Process()
  5.     {
  6.         // do something
  7.         if (Completed != null)
  8.         {
  9.             Completed(this, EventArgs.Empty);
  10.         }
  11.     }
  12. }
  13.  
  14. public class Subscriber
  15. {
  16.     public void Handler(object sender, EventArgs args) { }
  17. }

Now we will attach the Completed event of Published to Handler method of Subscriber to understand the clean up.

  1. Publisher pub = new Publisher();
  2. Subscriber sub = new Subscriber();
  3. pub.Completed += sub.Handler;
  4.  
  5. // this will invoke the event
  6. pub.Process();
  7.             
  8. // frees up the event & references
  9. pub.Completed -= sub.Handler;
  10.  
  11. // will not invoke the event
  12. pub.Process();
  13.  
  14. // frees up the memory
  15. pub = null; sub = null;

After the Process method has been executed the Handler method would have got the execution flow and completed the processing.  However, the event is still live and so are its references.  If you again call Process method, the Handler method will be invoked.  Now when we unsubscribe (-=) the Handler method, the event association and its references are freed up from the memory but objects pub and sub are not freed yet.  When pub and sub objects are assigned null, they are marked for collection by GC.

If we do not unsubscribe (-=) and keep other code AS-IS – GC will check for any live references for pub and sub and it will find a live event.  It will not collect these objects and they will cause a memory leak.  This common anti-pattern is more prevalent in UI based solutions where the UI events are attached/hooked to code-behind / view-models / facade.

Following these practices will definitely reduce your application’s footprint and make it faster.

You May Also Enjoy