Csdrhrt

TL;DR: Questions under the text

I am a software developer at the R&D center. Due to my seniority, I am oftentimes asked to review time/scope critical changes to our code-base. Due to the hectic state of the project, it is not unusual that I am asked to review others' code the day or even hours before a major delivery.

I often review pull requests from a contractor, Joe. Joe gets paid for every feature he completes rather than an hourly rate. It is my impression that Joe tries to get as many features done as possible, oftentimes skimping on documentation/code quality in the process. In the past, I have had to do overtime to fix issues with Joe's code, because once it's approved and paid for, he doesn't feel and nobody makes him responsible for bug fixing. Our team has pointed out that the situation is non-ideal, but the current managerial decision is that this won't change.

I would frequently request changes in Joe's code because it doesn't meet our guidelines. Joe would usually approach me with a story about how he wouldn't get paid if his code wasn't accepted in time. For a while, I would accept minor issues under the condition that Joe would fix them later, but that never happened. Eventually, I stopped accepting incomplete submissions and have asked Joe to escalate the issue if he's unhappy with my verdict.

Generally, Joe's code would have the following issues, in ascending order of seriousness:

1. Lacking documentation/formatting against our guidelines


2. Lacking tests


3. Architecture incompatible with what we were trying to achieve in the future


4. Performance issues


5. Flat out not working

Since Joe's features were often promised to the client in that particular week and there wouldn't be time for Joe to fix them, eventually other people have started approaching me to accept Joe's code, from Product Owner, through Project Management, my boss, to my boss' boss.

In the discussions, my project management/my superiors usually ask me to find a way to accept Joe's code anyway. I would generally let 1 and 2 through at this point but refuse to accept anything more serious, citing my responsibility for code quality and technical debt as an engineer. To put it bluntly, accepting code like this would cost us more time down the road than it is worth.

In some situations my superiors have asked to accept the code anyway, even if the issues were egregious, citing features promised to the client and the upcoming delivery. At this point I usually respond by saying:

• Anyone, including non-technical employees, can overrule my decision and merge the code.


• If they are my direct superior, they can send me an email ordering me to accept it and I will oblige.

I do this not necessarily as a CYA tactic, but rather believe that people think twice about the decisions they are making if they put them in writing. To this date, no one has decided to do any of the above, even if I have gotten some scoffs as a result.

Nevertheless, some co-workers have privately told me I wouldn't make it very far with this attitude, thus I wonder:

• As an engineer, code quality is my responsibility. To what degree should I fight for it?


• Is it appropriate for me to ask my superiors to put their decisions in writing?


• If code quality is suffering due to organizational issues (the way a contractor is compensated), is it appropriate for me to be "a judge" or raise the issue with my superiors or HR?

Adding an answer because the other answers dance around it but never really call out specifically in the shortest phrasing possible:

You need a "definition of done"

Generally accepted definitions of done include not only working code, but working unit tests, and documentation. Until those things are completed, the task is not done and the code cannot and should not be released.

While you'll often find the concept under the Agile/Scrum umbrella, that working framework isn't necessary to implement this idea.

https://www.scruminc.com/definition-of-done/

https://www.agilealliance.org/glossary/definition-of-done/

As I see it, it is not the policy that is in the fault, it's the person trying to abuse the policy.

As an engineer, code quality is my responsibility. To what degree should I fight for it?

As long as you are responsible for something, you should follow it until you can prove that any deviation is not caused by "you". Given the context you mentioned, it looks like you're fighting for the right cause. It's not only about the "coding guidelines", the target is to have a code that actually runs and produces the expected output.

Is it appropriate for me to ask my superiors to put their decisions in writing?

In this case, very much yes. You have given a negative review, and if someone asks you to waive off the blockers and allow that code to find it's place into the production code, make sure that can be traced back to the "ask", at a later point of time.

If code quality is suffering due to organizational issues (the way a contractor is compensated), is it appropriate for me to be "a judge" or raise the issue with my superiors or HR?

I would not say it's the HR who can handle, this should be informed to your superiors and technical managers who can understand the impact.

You have already tried some of the proper things to try in this scenario, however, there is one more thing I'd suggest:

Do not point out the person in fault, rather point out the problematic part of the code and also try to document the problems you can foresee if that code makes it way to the final version. Also, point out couple instances, where you allowed some of the minor cases which you allowed in the past, expecting that to be fixed in future but never done.

Seems like, the project management process is problematic. Sometimes, it's better to have partially completed feature instead of non-availability, but there's a silver lining. The point here is, the tracking perspective needs to improve. Just delivering the code (or issuing a pull-request) should not be the measurement for the delivery, it's the owner's responsibility to ensure that the code gets reviewed and merged to the production (final) code.

To sum it up:

• Ensure the review comments not only mentions the problem in the code, but also the adverse side effect which it will carry if allowed to be merged. Cite the rules/ guidelines based on which you are making those comments, so it becomes evident that you are not "nitpicking", rather raising valid concerns as per the well established guidelines.


• Try to set a deadline for the delivery which is well ahead of actual delivery time, so that there is enough time remaining for any change / fixes needed.


• Ask for the primary test reports to be associated with the pull request - that way you will have more proof to establish your points.

Finally:

Nevertheless, some co-workers have privately told me I wouldn't make it very far with this attitude,

Well, that's not a very good sign. Allowing bad code is not only bad for the quality, it's also an indication that those who are advocating the allowance, does not honor the time and effort put forward for the review process, and in turn, does not honor the work done by one or more employees. If this continues, you may better find an organization which values it's employee's efforts.

I think you do your job honorably, even if quite stressful. As a quality engineer, you should not allow very low quality work pass through.

The way I see it now, there are some alternatives - none of them perfect.

1. Convince your bosses to take responsibility in written (e-mail should be OK). The more bosses, the better.
   
   
   
   
   • one e-mail per incident;
   
   
   • one e-mail to rule them all; this pretty much cancels the quality activities in the project;
   
   
   
   


2. Give up your activities as a quality engineer, or at least the job of accepting / rejecting the work of others. You seem to have enough work already;


3. Convince management to let Joe be the one to accept / reject the work of the team. Whatever happens, Joe will answer - including his own work.


4. (worst case) Think of a different company which meets your expectations and start "transitioning" there.

Judging that both bosses AND your colleagues have a bad mindset about the need of quality of the work, option 4 seems to be your best friend.

I have had to do overtime to fix issues with Joe's code, because once it's approved and paid for, he doesn't feel and nobody makes him responsible for bug fixing.

This is the problem. You could make the best argument in the world about code quality but if his payment is based on features then that's precisely what he'll do. Afterall, why would he waste his time fixing already accepted code when he's not getting paid for it?

I recall a time in history when LOC (Lines of Code) was the primary way you'd get promoted or paid. You'd be writing all day for simple things. Very difficult to add features or even follow along.

Joe would usually approach me with a story about how he wouldn't get paid if his code wasn't accepted in time.

He even told you that he wouldn't fix it because he's not getting paid for it.

The first argument is to figure out how to fix this contract terms. Ultimately it is up to your business but your top 3 arguments are:

1. You're wasting time "fixing" what he broke. Are you being paid to fix his bugs or are you being paid for other things? I'd get clarification on that.


2. Paying by feature is broke. You can explain this in conjunction with #1.


3. If #1 is your primary hire, then you need to consider finding a new job. If they're unwilling to fix the terms, then you need to go regardless.

Try to make the accept criteria objective.

As an engineer, code quality is my responsibility. To what degree should I fight for it?

Yes, code quality is your responsibility, but that doesn't mean you need to make sure code quality is at the highest level. Your job is to make sure that code quality is at an adequate level.
High quality code can avoid bugs and improve maintainability down the road, but it usually comes with a cost (money or time), that is why it is not always what is needed, especially when you are not writing software for pacemakers or airplanes.

You write that you work in R&D and in that field it is very common to write prototypes that will be thrown away in 90% of the cases.

Finding the right trade-off between code quality and velocity and cost is hard and if it is not done centrally can easily vary between different developers.

If you are the one who is always saying no to new features that are needed by the business you will be primarily perceived as a troublemaker. Maybe 4 years down the road, someone will realize that you had always been right about pushing for quality, but it could be that you have been already let go by then.

The way around it is to have objective criteria for code quality that are backed up by the business stakeholders

Lacking documentation

Is all other code documented, or only Joe's code is lacking it? What is the impact of not having documentation?

formatting against our guidelines

This should be a clear case, you have guidelines (I assume they are sanctioned by company or department) that apply to everyone.
Sometimes however guidelines are outdated and nobody follows them. If other seniors follow them, you should be safe to insist on them.

Lacking tests

There is no global rule that code must have specific types of tests or have tests at all. Just because it is generally a good idea to them it doesn't mean it is a practice you enforce in your company.

Architecture incompatible with what we were trying to achieve in the future
Performance issues.

This one I find the toughest, as it is very hard to judge whether code architecture is good or bad.
But you can have two objective criteria:
a.) Are non-functional requirements met? (Only if you have defined non-functional requirements beforehand)
b.) Does the code fit in the overall system architecture (Only if you have documented system architecture)

Flat out not working

This is a no-brainer, nobody should criticise you for non-working code. Unless you go overboard and require perfect code, where good code would be sufficient.