Hello again, I’m in a situation where the one the senior devs on my team just isn’t following best practices we laid out in our internal documentation, nor the generally agreed best practices for react; his code works mind you, but as a a team working on a client piece I’m not super comfortable with something so fragile being passed to the client.
He also doesn’t like unit testing and only includes minimal smoke tests, often times he writes his components in ways that will break existing unit tests (there is a caveat that one of the components which is breaking is super fragile; he also led the creation of that one.) But then leaves me to fix it during PR approval.
It’s weird because I literally went through most of the same training in company with him on best practices and TDD, but he just seems to ignore it.
I’m not super comfortable approving his work, but its functional and I don’t want to hold up sprints,but I’m keenly aware that it could make things really messy whenbwe leave and the client begins to handle it on their own.
What are y’alls thoughts on this, is this sort of thing common?
It shouldn’t be up to another engineer to fix their PRs. They wrote the code, they are responsible for making sure it is in a state to merge. If it’s not, request changes and move on to your work.
Yeah, we comment in places where we see issues, leave tasks, and just mark the PR as “needs work.” I ain’t touching code in a branch that’s not owned by me.
Soooo much. It’s the biggest of red flags to have to fix other’s reviews.
Maybe take it up with managment. Those kind of profile are a hinder for every other devs that are working with them
breaks tests
leaves me to fix them during approval
I’m sorry, what? If he broke it, he fixes it. There should be guard rails that prevent him from merging his code until all the tests pass, and you as a reviewer should refuse to even start a code review unless the build is green.
Exactly, don’t even strat looking at a PR that doesn’t pass the CI pipeline
Im quite happy to help fix the tests with someone that is willing. This kinda guy, not so much
I don’t understand why you’d be fixing unit tests he broke during his pr. It seems like he might be bullying you? Maybe discuss with your manager.
Unless it was directly caused by some code he wrote earlier that wasn’t caught at the time, he shouldn’t even consider that
I’m not super comfortable approving his work, but its functional and I don’t want to hold up sprints…
I know it’s not the point of your post, but this is a red flag to me. If you’re using scrum (which it sounds like you are?), a sprint isn’t defined as “when all the stories get to done”, it’s a set block of time (generally between 2 and 4 weeks). If the stories don’t get to done in the time period, you don’t hold up the sprint - they just didn’t get to done. Most teams will just refactor the story into smaller pieces to carry over to following sprints.
Yes, it’s common. No, it shouldn’t be tolerated. Your boss/tech lead/whatever should be involved. Here is what should be done ideally:
- not following best practices: you MUST implement merge requests (GitLab, GitHub, etc.) and his code shouldn’t be approved which means that his code won’t ever be merged in a shitty state. Force 1 or 2 approvals for each MR, and it should not be possible to merge an MR if it has open comments. The boss should ask every day “why is your code not merged yet?” and he’ll have to explain why people don’t approve his shitty code.
- shitty unit-tests: same thing, the boss should show him how to do this, and the MR shouldn’t be approved.
- breaking unit-tests: it’s the job of the CI to literally block MRs that break unit-tests (whether it’s code coverage or unit-tests).
- leaves me to fix it during PR approval: NO, it’s HIS merge request, not yours.
To sum it up: devs must not approve his MRs, the CI must block MRs that break tests.
Last point is SO painful… I have a coworker that writes so much shitty code OR it straight up doesn’t work… he once submitted a PR that didn’t work when used! Like, I just started the thing and it was utterly broken - both the implementation and the design.
More so, some of his PRs are a giant nightmare of over engineered crap that he, at some point, doesn’t even understand.
Worst of all, he gets angry at me for pointing out that either they don’t work or they are a shitty, complex, mess
Honestly, at some point I started approving his PR and calling it a day; oh we don’t have tests cause reasons, I tried to use TS too but since my boss finds it too complicated we are not using it again for new projects… funny
This thread makes me so grateful for working with competent people.
just yesterday I had to drop a lot of his commits cause they broke some core functionality lol
This sounds like the story of how I got into CI/CD ….
I was working for a startup, and leadership was amazed at how “productive” this one developer was, changing thousands of files in marathon coding sessions. I realized he knew how to use the refactoring functionality in his IDE. More importantly, I was the QA peon tasked with being first one in every morning to characterize stability before everyone else started working. This guy and his antics made my life miserable.
However, I created build and test automation, and scheduled it nightly (this was before CI was a thing, before we had good tools), so now everyone got emailed the state of the product first thing every morning.
This diva was the only one making changes so all the instabilities were owned by him. Made my life much easier and helped our product get much better. More importantly, it did knock him down a peg, as the side effects of the scope of hos changes became more clear
I’m that dude on my team when it comes to SQL. Everyone on my team (in my whole company, it seems) follows zero formatting standards when writing SQL code. Nothing qualified, no indentation, no query hints or anything to optimize. People just get their sprocs and views into a state of “good enough” and just hit submit.
I’ve been dealing with similar things over the last few months.
Went through an org restructure, joined a different team and our tech lead was relatively new to the company but has 20yrs experience, sounded good!
Wrong.
Poor communication skills, endless pointless wrappers over everything and even simple things are wildly over-engineered with zero documentation, poor naming and zero tests to verify his homebrew solutions actually work or not.
We go live in 2 months for a nation-wide release. Send help.
I wonder, what kind of wrappers? I have seen some wrappers that seem useless at first, but they shine when we do a refactor because the wrappers concentrate logic in one place.
the pointless and poorly named kind.
Here’s one example of many:
class Group extends StatelessWidget { final CrossAxisAlignment crossAxisAlignment; final List children; const Group({ super.key, this.crossAxisAlignment = CrossAxisAlignment.stretch, required this.children, }); @override Widget build(BuildContext context) => Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: crossAxisAlignment, children: children, ); }For those unfamiliar with Flutter, Column is your go-to out of the box widget that’s already flexible and simple enough to implement and customize.
Groupexists purely to set a cross axis alignment value, and by name provides no indication as to what it does or how it lays out its children.All that just to save one line to override the default cross axis alignment…
He is either over-worked and doesn’t have time to abide by team standards, or lazy. If lazy, reject the PR for failing to meet standards.
If over-worked, he needs to talk to his manager or whoever the work is coming from and tell them they need to slow it down
I’d start rejecting his PRs lol, why is anyone but the original developer fixing his PRs?
Doesn’t sound like a senior dev to me. Sounds like someone who thinks they are.
My opinion: don’t sweat it, either way. I know that’s easy to say from the outside, but it’s still true. Do what you are most comfortable with. It sounds like you have plenty of ammunition if you want to put your foot down & insist on quality practices. Reject PRs that don’t meet best practices, and point to the internal docs you have. If the dev reacts angrily, blame the company & say you are worried about getting in trouble.
Or if confrontation makes you more uncomfortable, just let it slide. If the shit hits the fan, the senior dev is the senior dev. Just say you were following their lead.
Above all, remember that the company you are working for is not your friend and not your ally. Look out for your own interests first & don’t stress about work as much as possible (I get that’s easy to say and tough to do, but it’s still the best idea!)
It’s easy to become a 10x developer when you skip 90% of chores and let other devs doing them for you. If you keep enabling them, they’ll become 100x developer soon enough.
If you’re following agile it is important for a team to agree on a definition of done for a story. If you don’t have one ask the scrum master to start that conversation or bring it up in a retro. One of the things that everyone can usually agree on is that the tests pass. Throw in a minimal coverage threshold as well. It’s not an indicator of good tests but it will tell you when there isn’t enough.
You mentioned that you’re doing this work for a client and that they will take over the code. Verify with management (in your company) if there are any quality measures specified in the contract. You don’t want your guy not performing up to the client’s expectations and you having to put in a lot of last minute nights and weekends to get there.
I’m going to play devil’s advocate for a moment.
following best practices we laid out in our internal documentation
Are you absolutely sure those “best practices” are relevant or meaningful?
I once worked with a junior dev who only cared about “best practices” because it was a quickly whipped document they hastily put together that only specified stuff like coding styles and if spaces should appear before or after things. That junior dev proceeded to cite their own “best practices” doc with an almost religious fervor in everyone else’s pull requests. That stopped the very moment I made available a linter to the project, but mind you the junior dev refused to run it.
What’s the actual purpose of your “best practices” doc? Does it add any value whatsoever? Or is it just fuel for grandstanding and petty office politics?
his code works mind you,
Sounds like the senior dev is doing the job he was paid to do. Are you doing the same?
It’s weird because I literally went through most of the same training in company with him on best practices and TDD, but he just seems to ignore it.
Perhaps his job is to deliver value instead of wasting time with nonsense that serves no purpose. What do you think?
