After seeing this sort of issue come up in many projects in the shape of hard to spot bugs I decided to quickly write a small entry as a sort of OOP Commandment.
Thou shalt not return references to internal fields!
If you took the trouble of hiding an internal field making it private or read-only don't send it all to waste by handling it to the first caller of your class method.
A simple example of what I am talking about is as follows:
|
using System;
using System.Collections.Generic;
using System.Text;
public class MyObject
{
string m_Name;
public string Name
{
get { return m_Name; }
set { m_Name = value; }
}
}
public class MyObjectHolder
{
private MyObject m;
public MyObjectHolder()
{
m = new MyObject();
m.Name = "MyObject";
}
public MyObject GetMyObject()
{
return m;
}
public string GetName()
{
return m.Name;
}
}
class Program
{
static void Main(string[] args)
{
MyObjectHolder h = new MyObjectHolder();
Console.WriteLine(h.GetName());
// The private member now is not private anymore...
MyObject m = h.GetMyObject();
// Any changes in m will affect the original internal field of h.
m.Name = "SomeOtherName";
Console.WriteLine(h.GetName());
}
}
|
There are at least three options for this simple case. One is to turn the internal field into a ValueType, a struct in this case will fullfil the objective of holding the data.
The second would be create a temporary local variable inside GetName and assign it the same values for every field. The easiest way to get that is to make your object implement ICloneable (with a deep copy implementation).
The third option is to return an interface to the object with limited capabilities.
Here is a possible implementation:
|
public interface IMyObject
{
string GetName();
}
public class MyObject: IMyObject
{
string m_Name;
public string Name
{
get { return m_Name; }
set { m_Name = value; }
}
public string GetName()
{
return m_Name;
}
}
public class MyObjectHolder
{
private MyObject m;
public MyObjectHolder()
{
m = new MyObject();
m.Name = "MyObject";
}
public IMyObject GetMyObject()
{
return m;
}
public string GetName()
{
return m.Name;
}
}
class Program
{
static void Main(string[] args)
{
MyObjectHolder h = new MyObjectHolder();
Console.WriteLine(h.GetName());
// The private member now is not private anymore...
IMyObject m = h.GetMyObject();
// Any changes in m will affect the original internal field.
//m.Name = "SomeOtherName"; <-- error : does not contain Name.
Console.WriteLine(h.GetName());
}
}
|
Although String is also a reference type it is also an immutable type so it’s ok to return a direct reference to the internal string field. Any attempt to modify it will create a new string.
A caveat of the interface solution is that the client of your class can still guess the real type of the interface reference returned and force a cast. But then again if a developer goes through such an effort he deserves what he gets.
That’s it. I hope that helps you to avoid a couple of bugs here and there.
