Don’t do this

Hi friend.

Today I set out to extend an existing JavaScript class inside a client's project. Upon scoping the extension and figuring out where it fits in, I noticed the missing tests. You see, it would be reckless to extend the class without any test coverage. I will be gone from this client in a few weeks and I cannot leave them with code like that. Anyway, the class had no tests at all.

So I wrote a very basic test setup. This is an Angular project—which is neither a speciality of mine, nor something I would prefer. But with things being the way they are, I just try to add some value to the project. The Angular tests use Karma (as far as I know that is the standard for Angular CLI projects). I usually use Jest. But ok. The setup isn't soo different.

Angular uses dependency injection quite a lot (if people know how it works, at least). Even the tests require you to use it.
Sidenote: If I am wrong with any of these things, please tell me. I am learning as I am going with Angular. So please share any knowledge you have and that might clear things up.
So I create my test, provide the class I want to use and instantiate it with mocked values for the arguments that it requires. And I expect(service).toBeTruthy().

And the test fails.

The test failed when instantiating the class. Without doing anything else.

For the "simple" reason that the constructor didn't just setup the instance. No! It called one of the main methods of the class right there in the constructor already. If you imagine the class to be a gateway to an API, then it already did something like this:

// more code above...
constructor(things, it, needs) {
    this._foo = 'foo';
    this._bar = 'bar';
    this.getAllTasks().then(() => {
        this.doSomethingElse();
    });
    this.publishTimer = setInterval(() => {
        this.publish();
    }, 10000);
}

// ...
// down below
// ...

public getAllTasks(filter = '', limit = 1000) {
  // code to access the API and get tasks
  // return a Promise
}
// more code below ...

DON'T DO THIS!

Don't instantiate objects that already do stuff. For testing this For setting this test up, I already need to make sure the test is sync, because of the Promise.then() thing. And I need to make sure that I handle the interval in my test. And everything that happens in publish (which might publish to some event listeners or whoknowswhat) needs to be mocked. Just because I called new MyClass. This is bad code.

The author of the code would have noticed his bad design, if he had cared to write a test for it in the first place. If things are hard to test, that's a good hint that your design might be weak.

My next step will be to refactor this class to be able to test it and the extend it.
I initially estimated the extension to take around 4 hours. This seemed reasonable. But I knew too little about the system and assumed too much. I already ||wasted||spent two hours today to get this to work and to create a running test (again: Angular :evil:). Now the refactoring will take at least 3-4 hours. Then the basic tests will take a few hours as well. And then the extension will be possible. That's again around 4 hours.
Bad design, missing tests and little knowledge about JavaScript led to 150% increase in turnaround time.
Ouch.

Yours,
Holger

Similar Posts:

    None Found

Leave a Reply