0

I've been told that this function (or one very similar) is an example of deferred anti-pattern

function writeFile(file) 
{
    return new WinJS.Promise(function (complete, error, progress) {
        if (file) {
            Windows.Storage.FileIO.writeTextAsync(file, getEditorContent()).done(
                function () {
                    currentFile = file;
                    WinJS.log && WinJS.log("writeFile: file written: " + file.name + "", "MyApp", "info");
                    complete();
                },                      
                function (error) {
                    WinJS.log && WinJS.log("writeFile: error writing File " + file.name + "", "MyApp", "error");
                    error();
                }
            );
        }
        else {
            WinJS.log && WinJS.log("writeFile: error writing File: file object was null", "MyApp", "error");
            error();
        }
    });
}

I need all three branches of this to return a promise, as this method is part of a promise chain. If I want to avoid the dreaded anti-pattern, I should supposedly do this:

function writeFile(file) 
{
    if (file) {
        return Windows.Storage.FileIO.writeTextAsync(file, getEditorContent());
    }
    else {
        WinJS.log && WinJS.log("writeFile: error writing File: file object was null", "MyApp", "error");
        return WinJS.Promise.as();
    }
}

And move my done() code to the calling function. However, writeFile is called from a number of locations in my app, and so in the interests of DRY performs some functions after writeTextAsync. I don't want to move these to the calling function, so I'm left doing this.

function writeFile(file) 
{
    if (file) {
        Windows.Storage.FileIO.writeTextAsync(file, getEditorContent()).done(
            function () {
                currentFile = file;
                WinJS.log && WinJS.log("writeFile: file written: " + file.name + "", "MyApp", "info");
                return WinJS.Promise.as();
            },                      
            function (error) {
                WinJS.log && WinJS.log("writeFile: error writing File " + file.name + "", "MyApp", "error");
                return WinJS.Promise.as();
            }
        );
    }
    else {
        WinJS.log && WinJS.log("writeFile: error writing File: file object was null", "MyApp", "error");
        return WinJS.Promise.as();
    }
}

It seems like I've just swapped a wrapped promise for three returned ones. How or why is this better?

UPDATE

I think I just realised I can return a then promise that writeTextAsync returns:

function writeFile(file) 
{
    if (file) {
        // I can return this, right?
        return Windows.Storage.FileIO.writeTextAsync(file, getEditorContent()).then(
            function () {
                currentFile = file;
                WinJS.log && WinJS.log("writeFile: file written: " + file.name + "", "MyApp", "info");
            },                      
            function (error) {
                WinJS.log && WinJS.log("writeFile: error writing File " + file.name + "", "MyApp", "error");            
                return WinJS.Promise.wrapError(error)
            }
        );
    }
    else {
        WinJS.log && WinJS.log("writeFile: error writing File: file object was null", "MyApp", "error");
        return WinJS.Promise.wrapError("error writing File: file object was null")
    }
}
Community
  • 1
  • 1
roryok
  • 9,325
  • 17
  • 71
  • 138
  • 1
    Did you see the question about the explicit construction anti pattern in stackoverflow before? – Benjamin Gruenbaum Sep 14 '15 at 17:02
  • Possible duplicate of [How do I avoid the `Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572)? – Bergi Sep 14 '15 at 21:06
  • @BenjaminGruenbaum I presume you mean this one - http://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it - but in my case I don't wish to repeat my `done` logic, so I want it to remain in `writeFile`. I also want to log an error if no file object is passed. I'll try and put a simpler example into my question – roryok Sep 15 '15 at 08:34
  • @BenjaminGruenbaum actually scratch that, I think I just figured it out. see updated answer. – roryok Sep 15 '15 at 08:48
  • @roryok:Looks good - except that you want to use `Promise.wrapError()` (the WinJs equivalent of ES6 `Promise.reject()`) instead of `Promise.as()`, and that you'll need to re-`throw error` in your error handler so that the exception propagates like in your original code. – Bergi Sep 15 '15 at 15:25
  • @Bergi well then with rethrowing errors and wrapping things in promises, it seems a lot neater to use the "anti-pattern" I was first using, surely? – roryok Sep 16 '15 at 08:03
  • @roryok: Not really, no. There are more reasons why this is an antipattern even when it works, check out my answer on the linked question. – Bergi Sep 16 '15 at 08:21
  • @Bergi how do I rethrow the error? – roryok Sep 16 '15 at 08:40
  • @roryok: Just `throw error`. Or alternatively `return WinJs.promise.wrapError(error)` – Bergi Sep 16 '15 at 08:42
  • Btw, your original code has a real problem I just noticed: The `error` callback that rejects your new promise is showed by the `error` parameter of your error handler, and then you try to call *that*… – Bergi Sep 16 '15 at 08:44
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/89778/discussion-between-roryok-and-bergi). – roryok Sep 16 '15 at 08:45

0 Answers0