As was foretold, we've added advertisements to the forums! If you have questions, or if you encounter any bugs, please visit this thread: https://forums.penny-arcade.com/discussion/240191/forum-advertisement-faq-and-reports-thread/
Options
Pull requests and imposter syndrome
CelestialBadger
Registered User regular
Registered User regular
I’m a coder. I recently joined a new company and all was well for a few months, until we started a thing where we could no longer push completed features to master, but must now submit a pull request for comments from the entire team before merging.
This is not something I ever experienced before and it’s causing me a lot of anxiety. Every time I write a line of code I second guess myself. I think “What would x think of this? What would Y comment? It’s severely reducing my code output without noticeably increasing quality and 100% reducing my job satisfaction.
I was feeling pretty great about my code until this happened. It’s absolutely miserable but I don’t feel I can complain as I’d basically be saying “I can’t take criticism.”
Is this just the way the world is now? How does this process work at other companies?
This is not something I ever experienced before and it’s causing me a lot of anxiety. Every time I write a line of code I second guess myself. I think “What would x think of this? What would Y comment? It’s severely reducing my code output without noticeably increasing quality and 100% reducing my job satisfaction.
I was feeling pretty great about my code until this happened. It’s absolutely miserable but I don’t feel I can complain as I’d basically be saying “I can’t take criticism.”
Is this just the way the world is now? How does this process work at other companies?
0
This discussion has been closed.
Posts
for as much as you don't want your code looked at, many people also don't to be "that guy" commenting on everyone else's work
I tried not commenting on others work when this pull request thing started because I felt that it was bound to create office strife. But one co-worker I’ve had tension with in the past has started minutely commenting on my code. I find it hurtful, whether positive or negative comments. Makes me feel like I’m trapped in a game of office politics.
I never worked at a place with this method of code review in the past. It’s quite new to me. I feel that if someone has a problem with my code they should raise it with me privately, not pick it apart in front of everyone. But that’s not the process, of course.
Just start shouting about "VELOCITY!" and ignore the guy who is picking over your code because they're an asshole. You don't have to act on the feedback.
If you're afraid implementing something basic like code reviews is going to cause "strife", it's a sign of either excessive anxiety on your part or an incredibly unhealthy office culture.
The reviews are only visible to the team that's working on a specific project (same end executables, essentially) not everyone at the company or even everyone on the program (family of projects related by funding and some interfaces to each other). Anyone can merge on a pull request provided at least two people other than the author have approved it and it has passed the continuous integration build (the branch compiles and runs through all the unit tests with no issues). When we get a new member on the team we temporarely turn on the rule that says one of the reviewers has to be the software lead for that project. The only way to merge into master is via these pull requests, other branches can be merged into freely.
If we are doing something tricky that merits explanation as to why but doesn't merit being a code comment, we pre-comment our own pull requests to explain it to the reviewers.
If they're just having everyone do code review on everyone else and they aren't actually investing in the additional work to align expectations and resolve conflicts of opinion consistently then the whole exercise is a waste of time and I suspect very few people will actually do it rigorously. There should be a long term plan in place to eventually get everyone to a point where they can all look at a problem and come up with the same general solution with the same general pattern - even if it isn't their own personal super special way of doing it, they can all speak the team's "language" and code accordingly.
https://news.ycombinator.com/item?id=18035548
By-and-large failure and success are team metrics, not individual ones. Similarly, it's not up to any one individual to determine coding standards by themselves by mere fiat. The team should have had discussions on how they want to code and what standards they want to hold each other to - peer review isn't an excuse to be a dick to someone because they prefer tabs instead of spaces [insert Silicon Valley clip here]. And ideally the team receives very quick feedback from product/users/customers as to whether those standards hold up over the long term, and are constantly adjusting accordingly.
If nothing I am describing is taking place then whatever system your team has implemented is a giant waste of time and introduces yet another point of conflict for absolutely no benefit. But if people have had discussions about this, and there are things like codified working agreements and standards in place, and people are trying their hardest to both hold each other accountable while not taking criticism personally, then it will probably work as intended.
Perhaps this article will be helpful: http://www.thagomizer.com/blog/2017/06/15/you-are-not-your-code.html
I remember one guy thought he would be Very Smart by DRYing up a lot of repetitive code in various places and replacing methods that were basically copied and pasted. While he did DRY up a lot of code what he didn’t realize is that he had now coupled a bunch of unrelated things through his DRY code, and when their behaviors had to change individually his generalized methods were no longer suitable and a lot of his work had to be undone to create a proper separation of concerns. His pull request got through but in the end people realized it was a huge mistake. Fuck!!!
People get complacent and it’s not immediately obvious it’s a bad idea. It looks like well written code, reduces the lines of code in the overall project, but then one day you just wake up and realize it’s a nightmare.
Just because you have a process it doesn't mean everyone involved is working perfectly.
Pull requests increase chances of good code, they don't guarantee it. For all you know the reviewers of that request were tired, or under pressure to review 5 other requests plus work on their own code, or were lazy, or didn't see some part of it, or just plain didn't think of all the ramifications. Guess who else didn't think of all the ramifications? The author.
- "you keep declaring these ad-hoc, shouldn't we have a centralized place for this sort of thing". Me: "good point, we should, let me write up a new ticket for that since it's somewhat beyond the scope of what i was originally doing here."
- "if you use this functional interface here you can turn this weird loop into a stream that's way more readable" me:" good idea, let me update the request"
- "why are you doing this this way, looks strange?" Me: because the requirement says blah blah, but note the word "may". Them: "ok but what about blah blah". Me: "it is kind of ambiguous. Let's check with the customer". A couple of emails later and it turns out we are both wrong (but i was less wrong), and we found out what was right then and there instead of 6 months later from an end user.
- "this refactor actually changes the functionality of what's going on here if you pass on this specific condition". In some cases my response was "oh snap i hadn't considered that, thanks", and in at least one case it was "yeah i looked up what this was supposed to be doing and 3 years ago when we wrote it we were dumb. The new way is how it should have been all along". And we all learn something.
Yeah if you think that's bad, I'm sorry, but it's something you have to work on. Nothing in Smrtnik's post seemed out of the ordinary or rude.
As a person who most certainly suffers from “imposter syndrome” I’m very sensitive about my code and struggle with shame with every line I write. I don’t want to be on an emotional rollercoaster of constantly seeking validation from my co-workers and fearing their criticism. It is emotionally unhealthy for me and is causing stress. Perhaps I will get used to it.
I'm not sure what more to tell you.
Did you read the "you are not your code" link?
I have already been skimming their commits, and if anything they did severely clashed with what I did, I would have told them. But it never has.
I figure this pull request thing is one of those code fads, that come and go. It seems toxic, since it is shifting discussion from positive “pre-planning and discussion” to negative “blame”. It’s not “How should I do this?” (Neutral) but “Why did you do it that way?” (Can be interpreted as hostile, wastes time implementing things the wrong way.)
none of this seems particularly offensive, and my job uses the exact same process for config management
my team and the config management team also have a great working relationship as people though, so when one of them says “hey your syntax is bad, rejected” we know it’s not because they’re pissing in our Cheerios, it’s because what we merge affects literally our entire infrastructure
regardless of whether or not this process is or is not toxic, you have an underlying issue affecting your mental health and that is definitely real and detrimental so you should see someone to help work through it
i once spent a couple weeks writing something in C++ only to have the first review comment be "wouldn't it be better to move this over to the python side?"
I said shit, you're right, and redid it on the other side of the interface, throwing my work away. It wasn't hostile or blame seeking and I wasn't offended. It would have been better to catch it before I wasted time implementing it the wrong way, but once that ship has sailed, it's still worth redoing it the right way.
You get better code if you check each other's work, so that's what's best for the company. If other people are also bothered by the existence of code reviews, then there may be a culture problem at your gig that won't go away no matter how healthy your personal attitude toward code reviews becomes, and that's something that really your bosses owe it to you to fix in an ideal world. That said, I think your original instinct that you may want to work on taking criticism better is probably a good instinct.
That’s exactly what I’m talking about with this method of communication being a post-facto “blame” method of communication rather than a constructive “pre” method of communication. Better to communicate *beforehand* than rely on catching errors afterwards.
You may be a perfect human being who doesn’t mind wasting weeks, but I would be fuming.
You can do both.
In general, while this stuff can be scary, it can also be an opportunity to get everyone working together much better. That's only if leadership works hard to make it into that. If they aren't doing that, there may be a way to manage up to steer them in that direction.
Also, just in general, constructive criticism is a powerful way to make yourself into a better developer. The removal of the word "constructive" can make it feel scary and ego-bruising, but if you are so constantly afraid of failure or mistakes that you never want to accept external invalidation, then you are going to lock yourself into a stagnant place. A fire being lit under you may look scary, but there have been many times where similar situations have made me do things I didn't think I was capable of, and removed a LOT of fear for future endeavors.
One of the best ways to prove to yourself that you aren't an imposter is to make it through this exact type of adversity with your ego intact.
Our first game is now available for free on Google Play: Frontier: Isle of the Seven Gods
How big are your pull requests? If you are working on one thing for weeks it sounds like your scrum Master (or equivalent. Software lead) didn't break down work (with the team, in a planning meeting) far enough.
No daily standup meeting? Do you have tickets for each issue you work on that everyone involved comments on?
But none of those things are mutually exclusive with also performing regular peer review to ensure that work is being done, while in progress, to the standards that the team has agreed to before the work started.
It does sound like you have some challenges with anxiety which may make this kind of process difficult to work within. It also sounds like you are not familiar with this kind of working process, and that your company has either assumed you are or they themselves are also unfamiliar with it and are not implementing things properly.
At the very least, kicking off a code review process without getting everyone on board with what constitutes valid feedback is probably going to cause unnecessary friction. If you have concerns here then you could bring them up with your supervisor or the scrum master (assuming one exists).
That was me commenting on another post from a guy who didn’t pick up that he was implementing in the wrong language until someone commented on his pull request. He seemed to think this was a point in the favor of the system. I thought it was the opposite.
Man, that’s harsh. The “throw ‘em in at the deep end” view of swimming lessons.
Development can get this sort of macho “if you can’t take it you’re a pussy” attitude and I don’t thrive in that environment.
(That’s not an insult to people who do thrive in that environment of course: we are all different)
Peer review is intended as a non-confrontational process. The point isn't to get in a room and argue, or to make an escalating series of passive-aggressive (or just plain aggressive-aggressive) comments to each other on Git/Jira/Slack/etc. There are best practices that can be implemented which can help to ensure that the process is as constructive as possible. And as others have pointed out, if it does devolve into a toxic mess then that's more indicative of the culture than of the process itself.
For example, as brought up earlier in the thread, a common approach people use is to couch their feedback in the form of a question rather than as a statement of fact. "Did you consider using Python?" instead of, "You should've used Python" can get you to the same point while being received much more readily. Your issues with the timing of the feedback are correct (in that something this large should come sooner rather than later), but the truth of the matter is that sometimes things change or sometimes people miss things, and it's better to operate in an environment where you can bring up this kind of thing before it gets to the end user.