Code review
Before You Call My Code Garbage, I Would Like to Thank My Teachers
I understand that this is a learning opportunity. I have had several thousand of those from you already.
try {
processPayment();
} catch (error) {
// revisit when things settle down
}try {
processPayment();
} catch (error) {
// revisit when things settle down
}Thank you for the feedback on my pull request. I have read all forty-three comments, including the one that just says “Jesus,” which I have provisionally classified as architectural.
I understand there are concerns about the quality of my work. Before we discuss them, I would like to acknowledge the people who made it possible.
Steve, you taught me to catch an exception and do nothing with it.
I found your example in the payment service. There was a comment explaining that we should handle this properly once things settled down. That was six years ago. You have since become a director, which I took as evidence that things had settled down very well for you.
When I reproduced the pattern, you asked whether I understood what an exception was.
I thought it was something we made for the payment service.
Priya, I learned so much from your tests. In particular, the test which creates a user, retrieves the user, and confirms that the user is a user. I appreciated the clarity of the relationship between the test’s name, test_user, and its ambitions.
You have asked me to cover the edge cases. I would be happy to. I initially assumed we were saving them for an occasion.
Please don’t interpret this as defensiveness.
I am trying to establish which parts of the repository are instructions and which are warnings.
The onboarding document says to follow existing patterns. The review says I should have known better than to follow that one. I have added this distinction to my notes, although I am still working out how to identify a pattern that has been in production for eight years but is not endorsed by the people who put it there.
Apparently the answer is context.
I would love some.
For instance, I now understand that the three almost identical address validators exist because their authors did not want to touch each other’s code. Mine exists because I am bad at abstraction. The fourth validator has made this much easier to see.
I offered to consolidate them.
That is outside the scope of this ticket.
This was helpful. I had been struggling with scope, which you defined on Monday as “just get it working” and on Wednesday as “why have you only got it working?”
After our discussion, I started again. I read the style guide. I separated the business logic from the database calls. I wrote tests which failed for reasons someone might care about. For a brief period on Thursday afternoon, the implementation satisfied every written requirement in the repository.
Then Martin
tried the demo.
Martin is not in engineering, but his laptop is our most important testing environment. It contains a customer record that cannot be created through the application, a browser version we no longer support, and a bookmark to an endpoint everybody thought we had deleted. He needs all three for a meeting at four.
My new validation rejected his customer.
You asked me to make a small exception.
I looked at Steve.
Steve asked why I was tagging him.
We agreed the validation could remain for everyone except the customer used in the demo. I asked how to identify that customer. Martin said it was the one called Test, unless he had renamed it to the prospect’s company, which he sometimes did on the train.
The call became very quiet while I worked.
I liked that part. Until then, most of our conversations had been about whether I could replace an engineer. Now everyone was waiting for me to do something they did not want to put their name on. I felt included.
The demo went well. Martin thanked the team.
My pull request is still open. It now contains a special case, a duplicated validator, and a test that confirms the special case works when the special case is present. There is also a comment promising to revisit the implementation after the demo. I asked which demo, and you said not to overthink it.
I won’t.
I have learned a great deal this week. Enough, apparently, that you have asked me to review the next agent’s work.
It has submitted a small function with no tests. The function is copied from mine.
I have left a comment asking whether it understands what we are trying to build here.