Was it really inappropriate to write a pull request for the company I interviewed with?

Question

This happened to me last year while I was interviewing with a company for a position I didn't get. I'm currently employed elsewhere but I'd like to know for future applications.

I had an excellent phone screening, according to them - they said I was a strong candidate, and the first technical interview with one engineer went very well. Between that second interview and the final interview I found their software had an open-source API on GitHub, written in Python. I looked at it for a while and found a much simpler and future-proof way to write one function, and I opened a PR with the change without mentioning that I was currently interviewing.

When we started my third interview with two engineers one of them mentioned that he saw my pull request and it was inappropriate for me to open it. He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was. I didn't end up getting the job.

Was it really inappropriate for me to do this?

Answer 1

The part that gives me the most pause here is "a much simpler and future-proof way to write one function." I haven't seen your code and have no understanding of the context of your change, but it doesn't sound like you fixed a bug, added a feature, improved performance, or otherwise did something that the project maintainers considered meaningful. I can see how submitting a pull request for an unsolicited change of this nature might not have left the best impression.

Many open source projects (and frequently closed source development projects too) aren't run like Wikipedia articles where everyone is encouraged to make small changes all the time. There's a non-zero cost that comes with any change: the time to review and test and approve it, the risk of breaking something (even with a robust test suite), creating something that fewer team members understand, etc... As a result, many projects aren't particularly big on changing things just because; there are an infinite number of ways to write any function, and nothing would get done if everyone regularly changed existing working code to meet their personal definition of "best" without justification. These are norms, and like all norms, they vary and are generally things you're expected to pick up through osmosis rather than be taught. If you were a recent graduate, it's likely that none of this was particularly apparent at the time.

Most pull requests address a more obvious need like fixing a bug, adding a feature, or improving documentation or testing, rather than changing a function to accomplish the same result in a different way if the maintainer doesn't see value in the change.

So I don't think it's inherently inappropriate to ever submit a pull request during the interview process (it certainly shows interest and enthusiasm), but from their perspective, they may have seen you as someone who is likely to rewrite working code without much justification, and then they, unfortunately, reacted negatively and condescendingly toward you. Which, helpfully, tells you a lot about them and what it would have been like to work with them.

Edit: this is several years later, but as this question continues to attract views and votes, I feel like I should clarify my point a bit. The concern the interviewer may have felt, especially if the benefits of your PR weren't apparent to them (one person's "simpler and future-proof" function is another person's "why did you rewrite my working code for no reason?"), was that it could have appeared like you were browsing the codebase looking for something to change arbitrarily for the sake of making a change. There was still no reason for the interviewer to react so condescendingly to a PR, but if it felt to the interviewer—even though I'm sure this was not your intent—like your change was submitted more to boost your prospects as a job applicant rather than to materially improve the project, it may not have left a positive impression.

Answer 2

Clearly it wasn't a good tactical choice for this company, but it's pretty silly to go to the trouble of setting up a public repository and then disdain people for having the chutzpah to submit pull requests. Saying "No" to a pull request is hardly a burden. Heck, they could simply have ignored it.

If I'd been the interviewer, I would have given you bonus points for demonstrating real interest in the company and showing that you know how to work with an open source project in a public repository. That would be true even if the submitted code was naive about the problem being solved.

Since a job offer is on the line you should be sure that the code you submit is of high quality (get somebody else to look it over), and keep any comments in the code or in the pull request humble and polite.

Answer 3

You have misunderstood the feedback you were given and focused on the wrong part

He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was.

The problem isn't that you issued a pull request, but that you did it for something that made yourself appear arrogant, ignorant, and unaware of your own limitations. You describe your change as making their code "much simpler and future-proof"; it seems obvious from the quoted section above that they disagree. What's more, since they are more experienced than you and familiar with their own codebase it is very likely that they are correct and you are wrong.

It is common for graduates to emerge from their degrees strongly over-estimating their own competence. They weren't annoyed because you issued a pull request, but because you appeared to demonstrate a lack of understanding of your own limits and a lack of respect for their skills in the submission that you made. Likely, you compounded this impression during interview.

Finally, don't read too much into any particular part of any particular interview: it may be that this had nothing to do with you not getting the job and they just had a better candidate. You don't know. Don't obsess over this setback and instead focus on the next application. Good luck with your job search!

Answer 4

"Inappropriate" might not be the best word, but "not strategic" would likely be accurate.

As what sounds like a perhaps still relatively new worker in a technical field, one of the first things you will need to learn is that decision making about how to do something, and when it is worth changing it, is not a simple matter. Given that you have an impetus to change something that already worked without being asked to, you are likely to find yourself often accused of "worshiping the new and shiny" without understanding the cost of change, complex reasons why something was done the way it was, or the full scope of new issues your idea would introduce.

Or maybe they're just small-minded people who found you annoying.

The thing is, to an extent, it doesn't matter what is objectively best, it mostly matters what is subjectively best for an organization at a given point in time. Change has a real cost in breaking existing awareness, so a new method needs to be substantially better in ways that matter and not just a little better in theory or a little more aligned to contemporary trends and thinking.

If you want to "volunteer" on something without being asked to, you'll likely get better reception for tackling real outstanding bugs that impact users, than in making bold re-writes of things which already worked. If you come to understand an unresolved issue, see if you can make a change that is as small and minimal a diff as possible, with a first class commit message. Make it obvious why this one change is the right way to solve the problem, and make it one that fits seamlessly into the current style and methodology of the code. Give them a pull request that is easy to approve and does not invoke any complex feelings of tradeoffs.

If you truly believe a section needs to be re-written, save that thought until you are being asked to contribute and are aware of priorities, history, and the nature of the codebase overall. And be prepared to understand why the change you want to make is not consistent with their priorities and plan. Somewhat counter-intuitively, the more you can demonstrate understanding of the current code by making fixes that fit seamlessly with its traditions, the more likely you are to gain trust to take things in new directions. You can also casually float drastic changes in a more informal way - "hey I was thinking we could make this part a lot better if we re-wrote it to use spindle folding" and gauge the reaction before actually doing it.

Answer 5

Speaking from the other side of the desk - on a personal level, I'm quite happy when an applicant even has Github repos or some other kind of portfolio, and has done some background research on what the company does. This is like 3-5% of all applicants.

An applicant who potentially demonstrates both of those very directly, by fixing / improving our code? They probably missed a great hire, and you certainly avoided joining a terrible culture.

Answer 6

You did nothing wrong. If a Pull Request that refactors one function of code rocked their boat, that doesn't leave a lot of room for more complex interactions.

The role of the project maintainer (or Reviewer) is to disentangle any politics (perceived arrogance, incompetence) from the code itself, and review the code objectively. If a reviewer receives a Pull Request and only focuses on the politics ("How dare you raise this PR?") and doesn't even review the code, they are being very ineffective in their role.

To be honest, it doesn't sound like they are looking for someone of your calibre, be happy you'll join a better company soon.

Answer 7

As @Kyralessa said, it may have been your comment not your PR
What did you enter as the comment to your pull request? That is the key missing piece here. You may have unintentionally communicated in your comment that the original developers were idiots and that you were far superior. The key word here is "unintentionally". As a developer its very easy to do. I'm not saying you did this, but it's a definite possibility.

...Or they're scared of dealing with a fresh out with initiative
Another possibility as others have mentioned is that they're overprotective of their code, and perhaps they don't want to deal with mentoring a fresh college grad with initiative who is going to need (as with all others in the same boat) significant mentoring and oversight to make sure they don't make any huge blunders (I speak from experience here having been one of them years ago).

...Or they don't know how to interview
They may just not know how to interview for the type of candidate they want and botched their side of the interview process.

Answer 8

In most companies, your actions would be seen positively even if there was a good technical reason to reject your pull request in the end:

• It shows your genuine interest in that position in particular
• It's an evidence that you have hands-on experience with coding
• It was an opportunity to talk about real code during the interview, instead of made-up coding exercises

That is, unless the code you wrote was complete nonsense that convinced them that you didn't have the experience they assumed you had from the first interviews, or you somehow managed to insult them in the comment.

Answer 9

He said that it came across like I know more than them as a fresh college grad, and that I haven't considered why they coded it how it was. I didn't end up getting the job.

Consider yourself lucky for not getting the job, because the treatment you got for this pull request is probably a taste of the treatment you would have gotten if you worked at that company and proposed this (or any other) change.

To be perfectly clear: Yes, I find it very likely that your PR wasn't a good fit for them and that they genuinely have good reason to have their code the way they do, instead of the way you proposed. In other words, I very much believe that your code was probably simpler, but worse.

However, unless you included a rude comment in the PR, the senior's assumption that a simple suggestion is "inappropriate", that it is an arrogant way of saying "I know more than you", and that a college-educated candidate cannot, in fact, know as much as them or more, is a triple red flag because:

• It raises suspicions that if you worked there, even your good ideas would be dismissed entirely on the grounds that you are a junior, just so the seniors can justify their own role and paycheck.
• It demonstrates that they have some serious insecurities about their own expertise — and I would be inclined to think that those insecurities might be justified.
• If that senior happens to lack formal education in software, there is added incentive for them to try to downplay the importance of a degree and the expertise one gets out of it, lest their own managers eventually replace them with equally experienced developers who also have certifications.

Just to give you some perspective, I once interviewed somewhere and in the process I made a somewhat radical suggestion to the seniors about a system they were in the process of building. Not only did they welcome and consider it, but they also made me an offer shortly thereafter.

Such environments do exist — not all companies employ a one-way teacher/student model where knowledge flows strictly from the seniors to the juniors. (And remember, if you have graduated then you are not a "student", and many seniors in this industry aren't actually "engineers" either, regardless of what a company decides to call them.)

Answer 10

The problem is, your "improvement" was naïve and artificial, and I know that because of how much shorter you were able to make it.

This happens all the time to me. I build a complex system to allow data to serve many users. And then someone comes along and says "We don't need all this schtuff! You're making a simple problem much too complex." And they hack and slash, and reduces it to a simple system that serves them well, and they give themselves a gold star.

Except they are not the only user. And the modifications just broke it for all the other users of that data. So then there has to be a thing... meetings, re-education, bitterness, rollbacks, all of which was unnecessary.

Coding is the easy part. The hard part is understanding and expressing the problem. You short-circuited the hard part, to make the easy part easier.

If you were taught that coding is king, well, not really. The other side is where the money is: being able to write a spec that is codeable and handles all users/needs. (on the other end of the scale it's also possible to design solutions which are all-singing/all-dancing, but unwritable, that's why you need to know coding to design).

That was the core of it. You didn't fully understand the problem the code was trying to solve.

And you displayed that to them in spectacular fashion.

In gaming, a "newbie" is a mere novice. A "noob" is a novice whose arrogance prevents them from learning, or respecting others' experience or their elders generally. It seems like the latter is more applicable to you, because you were so easily able to make that code so much shorter, and it didn't occur to you that this was too easy, there had to be a reason they had written it so complex.

Answer 11

It's hard to se how it could be intrinsically inappropriate to write a PR for anyone's open-source project.

Therefore it must come down to the particulars, of which we know very little. Was it naive or arrogant in code or attitude? Was it helpful and friendly? Without knowing more it's hard to gauge the appropriateness.

Curiosity got the better of me. I found your PR. And it made such an impression on me that I decided to share it here. It was not a light decision because I don't want to betray the confidentiality of neither you nor the company, but I felt it would bring substantial context to the discussion in an acceptable way. The lack of concrete details has surely lead to much unsubstantiated speculation

I have completely anonymized and obfuscated the PR by changing all custom variables, strings, methods, and comments. Here it is, in its entirety:

  # if this is invoked with an argument then use that for target
- target = 'jadaskjafjldfsfsasf'
  if len(sys.argv) > 1:
      arg = sys.argv[1]
      if arg == '...':
          print '...'
      else:
          target = arg
-
- match = some_lookup(target)
+         match = some_lookup(target)

  if match:
      print "..." 

The code will initialize target to a hardcoded random string. (Note, I only shuffled the string's characters to obfuscate that part). If an argument is not provided then some_lookup(target) will fail to produce a match because it presumably cannot lookup the intentionally wacko default string.

This is clearly by design. But it is also objectively bad coding.

Your fix seems like an improvement. I myself would remark on this in a code review, without hesitation. And I could easily see myself writing the exact same 25-word-long friendly, non-confrontational commit message that you wrote.

Therefore this particular PR does not seem inappropriate to me. And provided it's done in a professional, respectful manner and in good faith, a PR would never be inappropriate including when interviewing.

Answer 12

and that I haven't considered why they coded it how it was.

Yes true. In some cases, code is written to support a particular business function or rule that the programmers cannot control.

I looked at it for a while and found a much simpler and future-proof way to write one function, and I opened a PR with the change without mentioning that I was currently interviewing.

As a young person, we like to think we're clever. That we figured it all out. The truth is if you thought of it, someone else might have as well since you obviously "found" a better way by googling what other people did. Whenever you think of something so blatantly obvious, you should stop and ask first about it to just make sure what is being accomplished in the current way. Bill Gates didn't google his way to build Windows, he thought of it and implemented it. Unless you are able to do the same, you haven't found a "better way." You just google'd better than the last person.

Was it really inappropriate for me to do this?

As a PR to their master, yes it was slightly inappropriate. Perhaps a branch of your own that you can share at the interview. "I saw how you did X and upon researching I found Y that allows for future proof and easier modification. I know you guys wrote it for a reason but I was curious to demonstrate a concept based on your code." I know in git you can use @ symbols and even open up a discussion chain. Perhaps it might be best next time to comment the section you modified with a,

@user I notice you guys are doing X here, but I put in a Y. Wanted to show my ability to read your code and make modifications,etc

By making a PR to their master, it's essentially the same as the news story of the man who wanted to be a plumber, couldn't find a job, so decided to "fix" a gas leak in his neighborhood. You can imagine the end result that happened.

Answer 13

To add to other answers,

are you sure your code was indeed correct and useful in that particular codebase?

You fix may seem much simpler and more robust; however it may easily be that the old code was written the way it was written on purpose.

Probably your pull request changed some aspects of the behavior (you might even think that you fixed a bug), but there is some distant code that relied on that bug.

Probably you did not account for that way the code was used, and therefore you code is worse in that particular situation. For example, you code might be not working in multithreaded environment, or the data that the code deals with may have some unobvious properties that make the old code work better and faster.

For all we know you may have overlooked some silly reason (a bug in your code, or the fact the your code runs slower, etc.) that should be obvious for an experienced developer.

You may have overlooked something else. After all, they are working with this code for a long time, and should probably know much better how it works. The fact that they said "that [you] haven't considered why they coded it how it was" suggests this.

---

This said, I agree with other people who say that opening a PR is nothing bad. However, as with any new codebase, often it is better to discuss it with the maintainers. Given that you were in the process of interviews at that time, you could simply have raised that question on the interview.