Monday, April 18, 2011

"I'll Whack Your Fingers"-Oriented Programming

Every now and then, a question about how to transfer data between game screens pops up on the App Hub forums. The source of the problem is that many try to solve this problem using synchronous programming. This does not work because game screens require interaction with users, which cannot be handled in a synchronous manner without freezing the game.

Although F# gives you tools to solve that problem using e.g. async workflows, most people are using C#.

How do you solve that problem in this language? Usually, people use a combination of events and public fields. This solution is fine, but I just don't like events much. They make bugs tricky to track, because I lose track of where handlers fit in the expected flow of execution of my code. Moreover, the only practical way that I know of to pass data from an event handler to the main code is through a shared variable (which I call "I'll Whack My Own Fingers"-oriented programming).

For this reason, I suggested to use Begin-End-style asynchronous programming. This requires an implementation of IAsyncResult if you want to do it in a nice .NET-ish kind of way. I am not aware of any such implementation (EDIT: Here is one, thanks mausch), so I wrote my own:

///  
  /// An implementation of IAsyncResult that wraps result data. 
  ///  
  /// Type of the result data. 
  class AsyncResult<T> : IAsyncResult, IDisposable { 
    public AutoResetEvent signal = new AutoResetEvent(false); 
    public bool isDone = false; 
    public T result = default(T); 
 
    public object AsyncState { 
      get { return state; } 
    } 
 
    public WaitHandle AsyncWaitHandle { 
      get { return signal; } 
    } 
 
    public bool CompletedSynchronously { 
      get { throw new NoneOfYourBusinessException(); } 
    } 
 
    public bool IsCompleted { 
      get { return isDone; } 
    } 
 
    public void Dispose() { 
      signal.Dispose(); 
    } 
  } 

Did you notice how I made all fields public? I think many a modern programmer would wince when reading this. What if some careless programmer accessed isDone and modified it? Bad things can follow, and surely we should restrict access to these fields using the private keyword.

I like to call this "I'll whack your fingers if you touch this"-oriented programming. Beside "private" and "protected", which were probably invented to tempt stressed programmers to replace them with "public", enthusiasts of this approach enjoy using "sealed" to punish users who might be tempted to reuse your code.

There are better ways to prevent accidental tempering with an object's internal state: access objects via interfaces.

For instance, here is the code for the Begin-End pair of methods that use my AsyncResult:

///  
        /// Open a MessageBoxScreen asynchronously. 
        ///  
        /// The screen manager to which the screen is to be added. 
        /// The index of the player who has control, or null. 
        /// The text of the message to show. 
        /// An object which can be used to wait for the request to complete and retrieve the result. 
        public static IAsyncResult BeginShowMessage(ScreenManager sm, PlayerIndex? player, string msg) { 
          var result = new AsyncResult(); 
 
          var screen = new MessageBoxScreen(msg); 
          screen.Accepted += (src, args) => { 
            result.isDone = true; 
            result.result = true; // User accepted the message box. 
            result.signal.Set(); 
          }; 
          screen.Cancelled += (src, args) => { 
            result.isDone = true; 
            result.result = false; // User cancelled the message box. 
            result.signal.Set(); 
          }; 
 
          sm.AddScreen(screen, player); 
 
          return result; 
        } 
 
        ///  
        /// Wait for the user to complete interaction with a MessageBoxScreen opened with BeginShowMessage. 
        ///  
        /// The object returned by BeginShowMessage. 
        /// Whether the user accepted or cancelled the message screen. 
        public static bool EndShowMessage(IAsyncResult r) { 
          var result = r as AsyncResult; 
          if (result == null) 
            throw new ArgumentException("Wrong type or null", "r"); 
 
          result.signal.WaitOne(); 
          result.Dispose(); 
          return result.result; 
        } 

Note how BeginShowMessage and EndShowMessage return and take respectively an IAsyncResult. Unless users of these methods really want to take risks getting intimate with AsyncResult, there is no risk of tampering. And if they really want to work directly with the inner parts of AsyncResult, why prevent them to do so?

I wonder, what's the point with "private" and "protected" anyway?

UPDATE:

Thinking a bit more on the subject, I think another overused feature resides in non-virtual public methods (and fields). Anything public in the API of a class should belong to an interface, with the exception of constructors and construction helpers (which maybe should be internal to assemblies).

I would however not advise to use abstract data types in all situations. Navigating in the call tree is a good way to learn a new code base, and typically abstract methods defeat this. One could say abstraction through interfaces is a form of obfuscation, as it effectively hides implementations.

6 comments:

mausch said...

Here's a complete implementation of IAsyncResult: http://msdn.microsoft.com/en-us/magazine/cc163467.aspx

Joh. said...

Thanks for the info, mausch! I wonder why they kept their implementation internal. Another case of "Here's something nice that's not for you!".

Arseny Kapoulkine said...

The class looks like it can be used from multiple threads. The implementation then has multiple problems - i.e. you're providing an IsCompleted property, but it's unsafe to access the result when IsCompleted returns true (write ordering); also the pattern for setting the value is the same in all cases (set result, set isdone, release signal). Abstracting it in a single function/setter simplifies the code and prevents possible errors in Begin*/End* functions. You can also assert that the value is only written once this way.

While I'm not a die-hard fan of encapsulation, in some cases it's *so* easy to screw things up that it's better to hide the implementation as much as possible.

Joh. said...

Arseny,

Regarding accessing the result after isCompleted appears true: This is indeed an unsafe access. However, note that clients must call the End method to get the result. Unless the End method makes improper use, clients should be safe. "Proper use" here means waiting on the signal before reading the result.

You have a point regarding the access pattern by Begin/End methods. Here again, interfaces can be used as an alternative to private-based encapsulation.

Jonathan Allen said...

> I wonder, what's the point with "private" and "protected" anyway?
They exist to make the public interface meaningful.
In .NET programming a class automatically has a set of interfaces. As soon as you start defining methods and properties they pop into existence. There use is primarily documentation, they categorize methods and properties by the type of code that needs to know they exist. These include:

Public Object: Things needed for polymorphic access to the class
Public AsyncResult: Things users of your class should pay attention to.
Protected AsyncResult: Things people who are subclassing your class should pay attention to.
Internal AsyncResult, Private AsyncResult: Things that no one other than the library creator should touch directly because you can’t use them safely or shouldn’t need to know they exist.

These interfaces are practically free; you pay at most the price of a single keyword. (They do however vary by the language of the consuming code. For example, VB and C# have different rules for how overload resolution works.)
There is a side benefit that they, unlike abstract interfaces, allow you to add additional members any time without the risk of breaking someone else’s code. But this is a minor detail if you are not shipping libraries.
> And if they really want to work directly with the inner parts of AsyncResult, why prevent them to do so?
Nothing. The beauty of reflection is that nothing is sacred and anything can be changed even if it is marked as private. The only restriction is that you have to really want to do it; you cannot call a private method and later say “oops, I didn’t know I wasn’t supposed to be using that”.
I see that you left the AsyncResult class as internal instead of making it public. In this case I would go one step further and make the whole class private, since the only thing that needs to know it exists is the two functions. Sure anyone can still see the real type, but they aren’t supposed to need to know that information.

Joh. said...

Jonathan,

You write that private and protected exist "to make the public interface meaningful". The point I am trying to make in my post is that you can achieve the same result using interfaces.

These two approaches, encapsulation by annotation and encapsulation by abstraction differ in a number of ways, which might be the subject of another blog post. Encapsulation by annotation is the solution people choose most of the time, but I have a feeling that in many (most?, all?) situations encapsulation by abstraction might be better.