DEV Community

Discussion on: Have you ever had a colleague who routinely gets undue praise for flawed code?

Collapse
 
joemcmahon profile image
Joe McMahon

First: if you updated the code and did not add tests for what you did, you were not refactoring; you were pushing code around. A real refactor takes the code as it is, puts it under test, improves it, and verifies (via tests) that the improvement worked. So from the most naive point of view, A took an approach that was better engineering, and you're going to have to take that as a lesson. If A added tests -- putting aside that they're not good enough yet -- A followed better refactoring practice, even if they made errors doing it. You need to look at that and own your mistake first: write tests. If you're not good at it, you need to learn how to do it better.

Let's put that aside for now. How do we address the situation, as described?

As you've described it, you have a set of code that doesn't quite work, but passes its tests. Why is this the case? If the test cases are inadequate, and you know what cases should be there, then you need to get this work into the team's queue.

Whenever a test fails to catch a bug, file a ticket. "foo() should return bar, but returns baz". In the comments on the ticket note that the tests do not cover this case, and the situation or data that triggers the bug. If they do, but the test passes, note that.

You've got to make sure that the failures are documented, explained, and the work that is going to be needed is in the backlog. You've got to make sure that necessary work to take old features is in the backlog. You've got to make sure that the work that's missing is in the backlog. Basically, you have to stop fuming about how it's awful and document it. If you can do code coverage for the tests, you should do that, partly because it will let you ticket untested code, and partially because -- if coverage is poor -- you'll have more grounds for your position.

Once you've got that in place, you need to remember that the point of all this is that the code should work. Not that you are right and A is wrong. Yes, it sucks that A is getting credit for things being great when what you see is different, but unless it's documented, you just look like a complainer who doesn't like A.

Once you have the situation documented, you need to talk to your manager -- I'm assuming you both have the same manager. First you are going to have to swallow your pride and admit that A's adding the tests when you didn't was a good thing for them to do, and that you should have had them in in your version.

Only after that can you say that the problem is that the tests aren't actually exercising the right code paths, because tickets A, B, C, D, ... all show that there are bugs that the tests are not catching. At this point you will need to propose a solution. And that solution is most likely going to be that the team is going to need to devote some time to getting these bugs fixed to improve velocity. And that will probably mean that you will need to work together with A to fix the bugs and make the test suite cover them.

I hope this is just a temporary interpersonal problem. If you work together some, it's possible that you'll both learn from each other. If you've been really careful and honest and neutral (which may be hard) about reporting the bugs, and frame it (to yourself too!) as "we're all trying to make things succeed, these are things in the way we can fix", then you may find it a lot less tooth-gritting to sit down with A and work through the tickets.