6

I started a new job about 2 months ago. I've been given a large amount of work and little to no guidance about the inner working of a very complex web app that is also tied to multiple IoT devices that have multiple hardware and software layers I am unfamiliar with. Aside from the usual issues of the company not managing scope well and me having to push back, I am being subjected to a "chip on their shoulder" code review of my work before it is accepted as done.

Now, I have many years of experience taking feedback from code review but these guys are relentless. They call out opinion based things like variable names, putting something in an existing module or creating a new one or force me to add additional scope of functionality that isn't in the original acceptance criteria before they will "sign off" on the merge.

Instead of being small iterative changes, pull requests drag on for weeks as scope creep sets in and bully devs keep finding new and nit-pikier problems with the code in their echo-chamber. This is causing me to make little if any forward progress on tickets and is making me look bad in the eyes of my team and manager.

I talked with my manager about this and she was very defensive of their existing process and said that I needed the buyin of the other developers before I could merge anything in.

Is there anything I'm doing wrong or is this just a crappy team with bad management?

  • 1
    I say stand up for yourself and push back when they try to kick something back for something like a variable name. If it's feature creep push back and ask for a new issue to work the new scope into. – BirdLawExpert Feb 1 '18 at 1:43
  • 17
    So, what do you think your team would post about you on Workplace? I'm not being snarky - just seriously suggesting that you think about how they perceive you. Would their post go something like "How do we get the new guy to stop thinking we're picking on him when we're just trying to get him to follow our process that has been working for us for years..." ? You may not be doing anything wrong, but it is always helpful to think about how what you're doing may be contributing to a situation. – ColleenV Feb 1 '18 at 4:36
  • 8
    Variable names are not always opinion based and area a valid topic to discuss during a code review. Sounds to me like you should be complaining more about the guidance you need for the project, rather than code reviews being too harsh. – ayrton clark Feb 1 '18 at 10:30
  • 5
    @Robert You don't think there is a problem with you coming in after only two months and telling the entire team that you know how to do their jobs better than they do? I know that sounds harsh, but that's probably how it seems to them. You've had a whole two projects go to market? I've had 10x that and would never walk onto a team and try to start changing things after only two months. Even if you do know how to do it better, the way you're going about it is counter-productive. – ColleenV Feb 1 '18 at 13:27
  • 3
    @Robert It's up to you to deal with it however you think is going to result in you being the happiest, so I'll make one more attempt and then I'll shut up. What if they are "dishing it back to you" because they think you're the intractable bully who won't even change a variable name to get along with the team? I've got a lot of experience with confrontations like this (mostly because I'm ALWAYS right lol) and giving as good as you (seem to be) getting isn't the best approach unless you want to spend your time at work stressed out and annoyed/angry. – ColleenV Feb 1 '18 at 13:49
9

You have to enter the system first to change it. If your way is not accepted, or even fairly evaluated, then that's the way it's gonna be. In time you'll establish your authority, stop being the new guy, and have your opinion heard more.

Your manager said you need the buy-in of other developers, which means you have to be accepted by the pack. It it what it is.

Now for some practical advice: Do they not like your variable names? Big deal, change them. Yes it's petty, but it keeps them off your back, and you don't have much political capital at the moment. Or if you want to mess with them, put in a ducky.

The one I'd be worried about is the scope creep. Do the other devs have the authority to expand the scope? In any case, every time this happens inform your manager by email (make sure to include links or IDs or whatever your CR system uses). The point here is to get a paper trail which explains your delays.

  • +1 for the paper trail. It is really important to show that the delay is not (solely) your fault. – Dirk Feb 1 '18 at 10:56
  • @rath: Somethings might be misconstrued as "scope creep" when they are things that should be known to be required. For example, if you are told to make a credit card entry form then it shouldn't have to be stated that it requires the CVV code or that the card number, date fields, name on card, etc should all be validated prior to submission to the actual card processor. It's impossible to tell if the other devs really have engaged in scope creep or if they are simply pointing out all the other things this dev should be considering in their code before calling it done. – NotMe Feb 14 at 20:37
4

Disclaimer: I am a software engineer, I do code reviews, and I am nitpicky like this. This is why.

Without knowing precisely the issues you're having, a lot of these comments make sense to me from a code review perspective. Good, expressive variable names are important to code readability. If I have a variable called x, you have no idea what it does. If I have a variable called myApiResponse, that tells you:

1) It is an object.
2) It is a response from an API
3) The API is (probably) called MyApi

These are good things to know when reading and following code flow and are important for code maintenance. If your variable names are being called out, it's probably because they don't contribute to code readability and are going to cause problems in the future with maintenance. These things matter.

Likewise, putting things where they are supposed to go is important. Let's say you have a large application, a part of which is processing orders from your customers. So you have a CustomerService, which handles customer data, and you have OrderService, which processes orders. Let's say you need a customer's credit card information (for whatever reason you're storing that in your dataset). But you put your method to access that information in your OrderService, because "after all", the order service needs it. But then, you have another service, let's say your customer service dashboard, which also needs customer credit card information. But the person building the customer service dashboard doesn't know that order service implemented this function already, so they check CustomerService (where it should be expected) and don't see it and they go ahead and implement it. Now you have 2 implementations of the same code that have to be maintained. Suddenly you've doubled the code maintenance load of your feature to get customer credit card info, which you could have avoided if you'd just put the functionality in the right place where it belonged.

These are important things and they need to get called out in code review so they can be fixed before the code is merged, because if they don't you'll have lots of problems later.

Remember: The code review process is your friend. There is a reason code reviews are done, and there is a reason why code reviews are gatekeepers to merge your code, because nobody wants to deal with bad code. So do what the reviewers tell you to do and don't complain.

One note about scope creep: Push back hard on this. If what is being asked is not in the ticket, your response should be that the work for the ticket has been scoped and pointed (assuming you're using Agile/Scrum) and shouldn't be modified. If they insist on modifying it, say that the ticket will have to be re-estimated and it may throw off the scrum plans for the team, and mention this to your scrum leader and your manager. Adding stuff to tickets that wasn't originally estimated is very bad and shouldn't be done at all.

  • I just wrote a very similar answer, but like yours better, +1. I agree that especially with newly incoming developers, this is important to keep up quality in a code base. I would deal with the scope creep slightly differently: I would argue that those requests should be new tickets, which the manager can then decide to shut down or approve. This will better show where the work is going - and make it easier to get at least some progress in. – bytepusher Feb 14 at 19:11
  • @bytepusher Of course, additional scope should be a new ticket. The OP says the scope is increasing for each ticket, and to that I say that shouldn't be happening and he should push back hard. Of course, in general scope creep is a managerial issue. – Ertai87 Feb 14 at 19:19
  • ah fair enough. Well, I upvoted anyway :) – bytepusher Feb 14 at 19:20
  • Indeed, these things can matter - and not just on the merits. Something individual devs often miss is that difference across the codebase has a cost, and being able to make additions or improvements which fit in seamlessly is a valuable skill. As for scope creep, it can be a bad habit - or it can be a business necessity, but where it is, a fixed sprint interval is not appropriate, because it has insufficient agility to meet an awareness of needs evolving daily or hourly. – Chris Stratton Feb 14 at 19:38
  • I agree with everything here except to say that I think the OP should be very clear about what is scope creep vs what is expected in proper code. For example, I would expect someone coding the OrderService to put in code that does validation on the order prior to saving it without having to expressly state that this needs to happen. Basically I expect the devs that work for me to use their brain on tasks. Maybe this means the dev needs to go get further details about the task, if so then ask those questions. – NotMe Feb 14 at 20:44
3

Whatever the reasons behind this process (and they could be many and valid) there is an unavoidable delay between you writing the code and it being accepted. You can't do much in the short or medium term to change this.

If you have some input into your deadlines then adjust your estimates accordingly. If anyone questions this just point out that it takes longer for your changes to be signed off as you are still learning the team's coding style.

If you don't have any input into the deadlines then talk to who sets your work or deadlines and explain the above to them. They should have the sense to schedule a bit of elbow room to mitigate any sign-off delays.

You haven't worked at the company long and are still learning a new team's process and standards (and how they manage requirements and scope). Some delays in getting code sign off is normal to begin with. You should find that this is lessened as you get some experience with the team and they get some experience with you and your coding style.

2

Code review criteria and development standards should be documented and thus repeatable by anyone with the same outcome. Ask for the documented development standards and adhere to these during development.

If they bring it something that isn't a documented standard for the company, recommend they have it added to the documented standard.

Regarding the scope creep, the time required to adhere to standards should be incorporated into your estimates -- if it's not, it's an issue with your estimation process. Always document anything that increases the development scope after your estimations have been incorporated into the schedule.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.