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.