Skip to content

Added async callback #20

Merged
merged 2 commits into from Jan 19, 2014

2 participants

@MicheleBertoli

The 'convertToHtml' method did not return anything because 'fileReader.onload' is asynchronous, so I added a callback.
Non working version: http://jsfiddle.net/MicheleBertoli/GFB7d/
Working version: http://jsfiddle.net/MicheleBertoli/GFB7d/1/

@lucaminudel
Owner

Hi Michele and thanks for helping !

Out of curiosity have you been able to reproduce the problem ?
I've tried running the jasmine tests in firefox, safari and chrome and they all work.

@MicheleBertoli

Hi Luca, tests work because you are stubbing the call: [...] spyOn(stubTextStream, 'getText').andCallFake [...].
Please, check the console on my fiddles.

@lucaminudel
Owner

You are 100% correct. When I tried this the event was fired synchronously while now it really is asynchronous.

The focus of the exercise is on the refactoring and on unit testing (i.e. this thread shows that an integration test is required here but is not here because the exercise has a different focus). Asyn programming is a challenge for many devs that should be better avoided here.

Because of this I'm leaning toward the idea of waiting that the event execution is finished instead of introducing the asyn approach:

        var fileReader = new FileReader();
        var text;
        fileReader.onload = function(evt) {
            text = evt.target.result;
        };

        fileReader.readAsText(this._fileBlob);
        while(!text) {}; // a comment to explain this hack 

This code is much simpler and similar to the code of the other languages. Whats your take on it ?

@MicheleBertoli

JavaScript is single-threaded so the while cycle is blocking the code to execute, please check: http://jsfiddle.net/MicheleBertoli/bf4Ef/

@lucaminudel
Owner

yep that's the downside, it freeze the browser until it has done (try http://jsfiddle.net/9Z6uT/ and use i.e. firebug to visualize the console). good to simplify the code of this exercise, bad DOM/javascript programming.

I understand that javascript devs are very comfortable to use callback functions just like you suggested. So making the change that you suggested is definitely and option.
Would you make that change to the original exercise (as you did) and also to the proposed solution https://github.com/lucaminudel/TDDwithMockObjectsAndDesignPrinciples/tree/master/TDDMicroExercises.ProposedSolution/Javascript/unicode-file-to-htm-text-converter and related tests?

That would be really helpful for all javascript devs.

@MicheleBertoli

I hope I have done a good job :-)

@lucaminudel lucaminudel merged commit 1d88bd2 into lucaminudel:master Jan 19, 2014
@lucaminudel
Owner

Hey Michele,

thanks for the improvement to the javascript exercise !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.