Saturday, August 24, 2013

Self-Inflicted Dangers of Multiple Method Operators in Dart

This may seem an odd confession, but I struggle with operator precedence at times.

I think this is because I don't have a background in computer science and/or have not played extensively with abstract syntax trees, which are a way of representing code in a data structure. It is also due to my obsession with “clean looking” code.

Tonight's adventure started simple enough. I wanted to return a future from the close() method of dart dirty that sent a File object:
   * Close the database, including the underlying write stream. Once invoked,
   * no more data will be persisted. If supplied, the callback [cb] will be
   * invoked with no parameters.
  Future<File> close([cb()=_default_cb]) {
    return _io.close()
      ..then((_) => cb())
      .then((_) => _db);
I cannot simply convert the method cascade (the double dot operator) into a regular method call and return it. The Future returned from _io.close() returns a RandomAccessFile instance (since _io is a RandomAccessFile). So I added the second then as a regular method that would return _db, which is a File.

My thinking above is to cascade the first then(), the result of which is the Future from _io.close(). In my mind, the last line ought to send another then message back to that same future from _io.close(). Now, there are better ways to accomplish what I am trying to do here, but I am setting those aside for a moment because it puzzled me when I tried to use the above code to delete the file in a test:
        then(expectAsync1((f) {
This failed because the Future contained a RandomAccessFile, not the plain-old File from the last line of my close() method:
  NoSuchMethodError : method not found: 'deleteSync'
  Receiver: Instance of '_RandomAccessFile@0x1da10ec4'
  Arguments: []
My problem is, of course, precedence.

In Dart, the method operator (the single dot .) has a much higher precedence than does the method cascade operator (the double dot ..). In my obsessive pursuit for clean code, I am unintentionally giving the impression that both operators have the same precedence:
    return _io.close()
      ..then((_) => cb())
      .then((_) => _db);
I have never given this sort of coding behavior much thought. In the other languages that I split method calls onto separate lines (JavaScript, Ruby) there is only a single method invocation operator. Most (all?) other reasons that I would split lines in those languages involve parentheses (multiple parameters, lists, hashes, etc), making the precedence obvious.

I stand by the need to write one method per line. I like to think of this as a 60 character limit on the width of code. However, you phrase it—60 characters wide, one method per line, one concept per line—the idea is the same: do not try to bury complexity in a single line. As deceiving as the above code it, at least it attempts to convey intent. Putting it all on one line is almost intentionally misleading:
  Future<File> close([cb()=_default_cb]) {
    return _io.close().then((_) => _db)..then((_) => cb());
In case it wasn't obvious the above is a way to solve my problem. Of course it is not obvious since there is so much information buried on that single line!

The fix above is to reverse the order of the then calls. The first then() is the future that I want—it contains _db, which is the File object that I have declared as the return type of my close() method. Next, I use a method cascade to call the optional cb() callback function. As a method cascade, its return value is its receiver, not its result. In other words, it returns the _db / File future.

If I am to retain this functionality, the question is then, how do I split my method calls across lines so I adhere to the one-concept-per-line rule? I suppose the best way is to further indent the cascade in an attempt to visually indicate precedence:
  Future<File> close([cb()=_default_cb]) {
    return _io.close()
      .then((_) => _db)
        ..then((_) => cb());
That is not entirely satisfactory. The diagonal indent is so strange that it catches the eye for the wrong reason.

Really, what my code is trying to tell me is that it is silly to accept a callback and to return a future. I need to deprecate the callback and switch to a future-only solution:
  Future<File> close() {
    return _io.close()
      .then((_) => _db);
In the end, I have learned two lessons. First, the one-concept-per-line rule remains inviolate. There is never a reason to break it and if I find that it is hard adhering to it, then there is some refactoring that my code it telling me to do.

The more immediate lesson is that I will never, never, NEVER split lines on different operators. The chance of introducing non-obvious bugs skyrockets. Methods cascades in Dart make it easy to perform many similar operations on a single receiver. If the messages being sent vary significantly, I am breaking the spirit of cascades—and quite possibly some of the precedence rules as well.

Day #853

No comments:

Post a Comment