joewalnes/jstinytest

Potentially unnecessary use of bind and apply

rickhallett opened this issue · 1 comments

Hi Joe,

First of all, thank you for sharing such an easy to understand testing tool! It's been a great in learning javascript and understanding the principles of testing, under the guidance and method of
@gordonmzhu at watchandcode.com.

After looking at the code in detail, I have some suggestions for improving the code. As far as I can tell, the use of bind() when creating test method aliases is currently superfluous. var fail = TinyTest.fail, without binding, would be sufficient as the variable has access to the necessary tinytest scope when invoking it's stored function.

Similarly, testAction.apply(this); can be run without apply, as we do not require the value of this to be passed through to the testAction function for normal operation.

So far, I have not been able to think of any use cases for bind/apply within tinytest.js. If the test methods themselves were to be modified in the future, and those modifications used this, it would already point to the tinytest object by default. In addition, I cannot think of a use case for why the API user would want to reference the tinytest object within test code itself; currently the tinytest code is protected within the scope of its API, and this seems to be an advantage to me.

It would be great to hear if you have any feedback on these thoughts. I would be happy to submit a PR for this small modification. The biggest benefit I can think of is having a less ambiguous code base for beginners - @gordonmzhu continues to use your tool for educational purposes.

Thanks also to @adilzeshan for helping me to test the above ideas.

You are correct. These are legacy from a very early version that passed around an object. PR would be welcomed!