Today's CodeGaffe is something I see all the time. It creeps into your code over time. Here's the scenario: Your class has grown from its humble beginnings, and there are now fields in your class that should not be used by some of your methods. In essence, you want some of your data to be private from part of your class. As in "really private".
If you've ever found yourself thinking, "Most of this class shouldn't access field foo,” then you've experienced this.
Here's an example I ran across recently - this class has two slightly different copies of an object, called a Thingee, but only one should be used by the majority of the class. Which one would you use if you were adding new code?
///
/// This class has a Thingee that it works with.
/// But, in reality it contains 2 Thingee objects,
/// and dynamically chooses which one should be used.
///
public class PrivatePrivateBad
{ private object OurThingee
{ get { if (m_CatIsAround)
return m_Thing1;
else
return m_Thing2;
}
}
private bool m_CatIsAround;
private object m_Thing1;
private object m_Thing2;
public PrivatePrivateBad()
{ m_CatIsAround = ...;
m_Thing1 = ...;
m_Thing2 = ...;
}
public void SomeMethod()
{ // I can access both fields, but really shouldn't
Console.Out.WriteLine(m_Thing1);
Console.Out.WriteLine(m_Thing2);
// Accessing the property is OK
Console.Out.WriteLine(OurThingee);
}
}
The most common solution to this problem is to dictate (in comments, where else?) that you must always use the property OurThingee instead of accessing the two fields directly. And that will work - as long as everyone follows the rules. But there's no way to enforce it.
Without enforcement, as the code evolves, eventually someone will not know about this convention. They'll access m_Thing1 or m_Thing2 directly. After all, both fields are private to this class, so it is reasonable for a new person on your project to use any of the fields as they add new code. And in a large class, once someone does this, future developers are much more likely to follow the same example and reference stuff that should have been "really private". This is how good code goes bad.
So, the intent of the original design gets lost. And bugs appear.
Fortunately, there's a simple way to enforce your design. You need to create another class. The new class will contain both Thingee objects, and hide the details of which Thingee you're supposed to use. It will simply have a property or assessor method to return the proper object. The new class will enforce our design, so we'll call it a ThingeeEnforcer, like so:
///
/// This class hides the details of how we dynamically
/// choose which Thingee should be used.
///
public class ThingeeEnforcer
{ public object CurrentThingee
{ get
{ if (m_CatIsAround)
return m_Thing1;
else
return m_Thing2;
}
}
private bool m_CatIsAround;
private object m_Thing1;
private object m_Thing2;
public void SetThingee()
{ m_CatIsAround = ...;
m_Thing1 = ...;
m_Thing2 = ...;
}
}
Refactoring our original class is actually pretty easy. You need to move the relevant fields to the enforcer class, and change the OurThingee property to access our new class. Wherever you set the initial values of the objects, you now delegate the work to the new enforcer class. Here's what the refactored original class looks like. Notice you can't possibly reference the wrong object anymore:
///
/// In this class, you can only access Thingee via
/// the ThingeeEnforcer, so there's no way to
/// get the wrong one.
///
public class PrivatePrivateGood
{ private ThingeeEnforcer te = new ThingeeEnforcer();
private object OurThingee
{ get { return te.CurrentThingee; } }
public PrivatePrivateGood()
{ te.SetThingee();
}
public void SomeMethod()
{ // This is the only Thingee we can access
Console.Out.WriteLine(OurThingee);
}
}
The steps for refactoring the Thingee stuff into a separate class are straightforward.
- First, review the original class's code and make sure all code references the appropriate property or accessor method.
- You're likely to see only two or three places that truly need to access the fields directly:
- The constructor (or some initialization method) that sets their values
- The property, or accessor method that gets the right value
- Possibly a method that clears their values
- Create a new class; we called it ThingeeEnforcer as it enforced the rules of access.
- Move the fields to the new class
- Add method(s) to set/clear the values
- Add a property or accessor to get the correct value
- Convert all code in the original class to use the new methods/accessors from the enforcer class. In our case, this was pretty simple since (in step 1) we made sure all the code was accessing our property. The good thing is, by moving the fields (in step 4) the compiler will help you find every single reference that needs fixing.
Now, when people add new features to your class next time, they won't be tempted to access the "really private" fields. They're safely tucked away in your enforcer class.
As I said at the outset, the key is to realize that as soon as you find yourself thinking, "Most of my class shouldn't access something" you've hit the point where you need another class to impose that restriction.
It's OK to create small classes like this to enforce some condition. That's exactly what object oriented design is all about. And your code will sparkle.
Technorati tags:
CodeGaffes,
Geekswithblogs