19

I am a developer with only a couple of years of experience, working in a small company of eight developers, on a Java project. Our team leader, also one of the managers, is older, and very productive, and says he has about fifteen years of experience in Java.

Last month, while we were discussing some source code modules he wrote, I noticed in the source code that one the classes overrode method equals(), but not method hashCode() - I am referring to the two methods declared in the very basic Java class Object. When I pointed this out to the team leader, in a very calm and polite manner, he denied that a class should override both methods. The issue would finish there, but I found it immoral to let such a flaw (bug) to hurt the product later.

A few days later, I approached him in person and in private and I quietly, without being disrespectful, explained to him about the issue, and used external references (such as Joshua Bloch's book 'Effective Java'). Well, he said he would look at it, and eventually he did. However, ever since, he gives me the cold shoulder.

Even worse, I recently saw a serious issue in the source code. Some classes he has written implement the Serializable interface, but the field serialVersionUID is not a fixed (constant) number. Instead it varies. I mean, we get a different number each time we run the application.

Again with the motivation of delivering a sane product, I would like to communicate this properly. But I do not know how to properly do so, without him taking it personally. You see, my previous approaches failed. How could I do so?

Any advice would be welcome.

Edit: The purpose of this question is to find effective, productive, polite, respectful, collaborative ways to communicate improvements over serious code issues, and not to question the authority or management decisions, made by peers, or even superiors.

Improve this question
20
  • 21
    What actual, rather than theoretical, problems are these issues causing in your code?
    – Philip Kendall
    Commented Mar 15, 2016 at 6:53
  • 16
    @Philip The problems described are those that may often not result in actual bugs, but if/when they do result in bugs they are usually difficult to fix. Even if that were not the case here, the question applies equally well to questions of code style and cleanliness; whether or not actual bugs have been identified is not relevant.
    – Rob
    Commented Mar 15, 2016 at 8:13
  • 8
    "I noticed in the source code, that one the classes overrode method equals, but not method hashCode." -- if you want to effectively communicate to a programmer why something like this is bad, find instances of actual bugs in the code that can be directly traced back to this bad practice. Then you can definitively say "if we adopt practice X, bugs of this type will not occur."
    – Brandin
    Commented Mar 15, 2016 at 9:08
  • 8
    @pek Best would be to find an actual demonstrable bug, file it in the bug tracker, fix it, and then point out. Hey, I fixed bug #1234. If we make sure we always do X, bugs like this won't happen again. I recommend we make it part of the coding standard from now. The rest is up to the team lead really.
    – Brandin
    Commented Mar 15, 2016 at 9:30
  • 5
    Having a different serialVersionID for each run is fine as long as the serialized objects are only reused within that run. If that is the case, it may be better if the versions change with each run.
    – kevin cline
    Commented Mar 15, 2016 at 21:31

12 Answers 12

Reset to default
42

General reminder: it's always better to avoid a critical or lecturing tone. Either make it a simple observation ("you seem to have forgotten to implement hashcode here") or make it a question ("is there a reason you didn't implement hashcode?"). Let their response drive drive the why-it-matters discussion, if necessary, but start from the assumption that they know the principles and that the oddity was either a simple oversight/typo or had some rational reason behind it which can be discussed rather than beaten upon.

People listen better if you treat them with respect.

And remember, sometimes the misunderstanding is going to be yours. You'll embarrass yourself a lot less this way than if you make a more absolute or didactic statement.

Improve this answer
3
  • The question approach is very good!
    – jwsc
    Commented Mar 15, 2016 at 10:47
  • 1
    @keshlam True, you are right in the three topics you covered in your detailed answer. Especially, the first paragraph, the idea to follow a "question based approach", opens a totally brand new perpective into the topic, and into communication methods, more general. Really, thank you very much
    – pek
    Commented Mar 15, 2016 at 13:15
  • Questions like "is there a reason you didn't implement hashcode?" work really well when dealing with intentional slackers; but they tend to backfire when dealing with people who feel overburdened (either because they struggle, or they have too much on their plate). It will put them on the defensive and is likely going to cause friction. The question inherently points out their mistake and asks them to justify it. It's phrased kindly but the required response is not simple and implies attributed guilt.
    – Flater
    Commented Jan 14, 2019 at 13:47
28

Maybe looking at this from the other perspective might give you a better view.

I am a team leader and also a manager in a small company of eight developers. I have 15 years of experience programming in Java. I have one new person in my team who is much younger than me with only a couple of years of experience.

Last month, while we were discussing about some source code modules I wrote, he started to belittle my code. Even though he has far less experience than I do, he claimed to see bugs in code which was working completely fine. After a short argument it seemed as if I was able to explain this to him.

But unfortunately he just can't rest that matter. He keeps undermining my authority by demanding that we do everything by textbooks (such as Joshua Bloch's book 'Effective Java') even in those cases where it would just be a waste of our time and not result in a tangible benefit for our product. How can I get this junior developer to trust my experience?

It is hard as a junior to convince a senior to change their practices. Also, as a manager he has to maintain an aura of superiority to be able to lead effectively.

So when you want to improve the quality standards in your workplace without the authority to do so, lead by example. Do right what others do wrong and show that your way is better because it results in a better product in less time. When you point out errors in other peoples code, don't do so by nitpicking at the sourcecode files. Do so by presenting a reproducible defect in the product itself and a way to fix it by following best practices.

By the way, one of the most common occupational disease for programmers is an inflated ego. He might have one already, but you are starting to show symptoms of the early stages too.

Improve this answer
8
  • 2
    Thank for your response, it is great in the way it tries to put me into his shoes. About the storytelling part: I really hope that the first discussion was really not perceived as an argument, and the quality concerns were not really perceived as "undermining ones authority". If not, there has been a really huge misunderstanding. For the rest, I think your approach of "show by good example" and "show by test cases" is very practical, scientific, and can eventually help building a better product and perhaps relationships... so, yes, I like it. Really, thanks a lot
    – pek
    Commented Mar 15, 2016 at 13:20
  • Wow. Preach it, brotha.
    – MealyPotatoes
    Commented Mar 16, 2016 at 4:57
  • 2
    @jamesqf The same interfaces are used in C# and although the code will will work in most instances (depending on implementation), not overriding Hashcode() will cause hash data structures to fail (Although everything else should work) and the same with serialVersionUID with causing some unwanted hard to debug issues. It's easy to miss overriding these methods if you don't fully understand the scope; something that pek expects his senior developer should understand.
    – Dean Meehan
    Commented Jan 8, 2019 at 16:19
  • 2
    "Also, as a manager he has to maintain an aura of superiority to be able to lead effectively" Nope. You don't need to be superior to lead: you need to be a good communicator who sets good examples for others to follow, by demonstrating commitment, passion, empathy, honesty and integrity.
    – bishop
    Commented Jan 8, 2019 at 16:33
  • 1
    @JoeBradley Too much importance? We don't know at what kind of software OP works, but if it's my bank, I'd rather he puts "too much importance" on such issues than end up with a few thousand bucks less in the account, because some operation didn't find some booking that transferred money to me and thus skipped it. Those are important issues, proper software construction is as important as proper bridge construction (arguably sometimes more sometimes less). I do agree that seeing it psychologically from the other side might help get the tone right in the conversation though.
    – Frank Hopkins
    Commented Jan 11, 2019 at 13:25
6

A way to mitigate against this sort of problem is 'give and take' I've been successful with this strategy many times. People are more amenable to your suggestions and criticisms when it's two way. So I would ask questions and advice when I got a chance and take their answers seriously. (To be honest sometimes I already knew the answer, but you never know, I've also learnt some cool things that way as well.)

Eventually you build a rapport where you support each other and things go a lot better in many ways. And if he's got 15 years experience, I bet there is a lot he could tell you that's worth knowing.

I wouldn't worry about him and his cold shoulder, that is normal enough after what happened. In all likelihood he will have taken your input onboard and is being careful around you, and you probably have upset him a bit. But it's not a popularity contest, so unless he gets out of control over it, give him some space. Remain friendly and supportive and he'll come around.

Improve this answer
1
  • Good point, good strategy! This may lead to a stronger bond (rapport) indeed. Thank you!
    – pek
    Commented Mar 15, 2016 at 9:20
2

Be polite and professional. Submit a ticket for the bug, in your ticket, detail the problem and a potential fix, and leave an opening for you to be contacted for more information or an explanation of the problem. Leave it at that. When your tickets are examined, it will be assigned to either yourself to fix the bug you found, or the original developer. Hopefully the other developer will understand and fix the problem, or come back to you for more information. If neither happens, then it's a management problem now.

This method doesn't call out any specific developer for mistakes, and allows repair time to be accounted for and possibly even included in a scrum (if you do that).

Finding bugs in other peoples code is common practice. Every time a new developer enters a project, they find issues. Assigning blame is not important. Just make a ticket, and let the company processes handle it. Experienced developers shouldn't take offense to others finding problems in their code. And if they do, they probably shouldn't be developers.

Improve this answer
2

While I think Keshlam makes a good point. It’s also important to remember that the team owns the code, not any individual, so I’d avoid the use of “You” when you ask about potential issues with the code. You could instead ask

“Hi, I noticed we haven’t implemented the hashcode here, is there a reason for that?”

You’re addressing the potential issue with the code, but also enquiring about a possible reason for the issue, and giving the team leader an opportunity to make it into a learning experience for you.

Improve this answer
1

Instead of making this a "I think you should" discussion which by now is clear that won't get you anywhere, then make it into a technical discussion.

After reviewing the code I found a problem and I wrote test X that demonstrates it.

(You can write several if needed). It is then up to the team to discuss how to properly fix it so your test succeedes.

Improve this answer
1

Do you have morning standups? or some other team meetings where you can mention it informally.

Raising these to everyone in a completely impersonal "Oh bother I found this issue this class has implemented equals not hashcode, that can really cause problems if the class is ever used in a hashmap as only object identity is used as the key" and then discussing when it can get fixed.

You have not accused anyone of writing bad code, everyone now knows why this is a problem, the code gets fixed.

Improve this answer
0

A few general pointers, though from this information, I think points 4 and 5 might be most appropriate:

  1. Seperate the code from its author. You are here to assess the former, not the latter. 'This code is [X]' is better than 'You made this code [X]'.
  2. Be specific. Do not give vague descriptions for the entire codebase. 'Descriptive comments would be appreciated at X, Y and Z regarding the wording of this function' is infinitely more appreciated than 'It's all Greek to me.'
  3. Be positive as well! the term for this process is 'code review', not 'code verbal-butchering' for a reason. In most published reviews, a reviewer will talk about good aspects of a product as well as bad. You should do the same.
  4. Pick your battles. Don't treat everything as a deal breaker. If it's a bit weird but it works perfectly well, give it a footnote at best, and address it as a 'minor issue' or 'quick question'.Generally, speaking, if you have to open up a debugger/decompiler to create a breakage scenario, it doesn't count as anything serious.
  5. Make sure the things you talk about aren't just 'in theory'. Test the application out yourself or talk to the testing team about something you think might be an issue.
Improve this answer
0

Write a test case, describing the behavior which you expect and show that the problem breaks the test.

(O boy, what insane things we discovered when we started to test hashCode and equals systematically in our project.....)

Improve this answer
0

Pick your battles

As developers, it can be hard to let small details go. Really hard. And as a junior dev, you often don't yet have the experience to distinguish between major problems and minor nits. It sounds like OP is right in the two examples given BUT are they major issues? To be honest it doesn't sound like it unless it's causing something to break. Not every code smell is a hill to die on, especially...

Tread lightly with superiority differentials

...when you're junior and they're senior. For three reasons:

  1. Bosses (assuming senior dev has authority over OP) don't usually like being corrected by their direct reports. Not saying it should be that way, but it's human nature.
  2. Senior devs (myself included) realize that they have to work harder to keep pace with the younger devs when it comes to their knowledge and tech skills. They can be very sensitive to anything that reinforces that fact to them like being shown up (especially in accidentally condescending ways) by junior devs. Again not saying it should be that way, but people are people.
  3. Senior devs are often used to dealing with junior devs who have a chip on their shoulder: who think they're brilliant and that senior devs are fools and posers, and who are always looking for opportunities to make this very clear. So even if you aren't one of these junior devs, if you appear to be doing what they do, you may get the same reaction that they get: defensiveness and extreme displeasure. Again, this isn't ideal, but put yourself in the senior dev's shoes.

Or possibly the senior dev could just be a jerk. Or have had a bad day. Or something else.

What to do next time

  1. Remember that the senior dev is senior, so pick your battles, and accept that as the decision-maker the senior dev has the right to make (and own!) mistakes.
  2. Frame your challenges as "I have a question so I can understand", not "you did this obvious thing wrong" so the senior dev can save face. This will allow them to (hopefully) see their error and fix it while preserving the relationship. If they don't see and fix their error, refer to #1.
Improve this answer
-2

Super good question. I love learning but when someone harshes my mellow I feel like I should quit. I recommend reviewing your coworkers "coding style" and identify his strengths so he doesn't feel like all that time he spent learning was pointless. THEN identify OPPORTUNITIES. That way you display strengths going forward that are helping the company and also working on those "opportunities" that you have already identified. I lead teams by giving a 30-60-90 day timetable and then praise the tweaks where we capitalized on "opportunities". It works very well and is a fair way to identify leaders vs people who are not and should not be leaders.

Improve this answer
3
  • Well, thank you for answering, but I am not sure I comprehend you. Just for making it clear from my part: I am not questioning ones leadership. Neither wish to take ones leadership role, either.
    – pek
    Commented Mar 15, 2016 at 13:29
  • Someone has to lead. I'm speaking from personal experience. I didn't mean to confuse you.
    – HotBreakfast
    Commented Mar 21, 2016 at 15:10
  • 1
    I pity the fool that “harshes your mellow.”
    – Festus Martingale
    Commented Jan 10, 2019 at 1:23
-2

Fix it yourself. Create a branch, fix it, dev test it, and if you need approval, then just do a code review. It changes the burden to you, but you're the one who thinks it's important, so the burden really does fall on you, even though it's someone else's code.

Improve this answer
4
  • The only problem with this approach is that if the product breaks, and it traces back to this fix, then the OP loses credibility since his fix might be a theoretical "best practice" one and not an actual fix.
    – Dan
    Commented Jan 10, 2019 at 19:31
  • @Dan, agreed. He had better make sure he's right. That's why I included dev test it in the steps. If he thinks it should be done, he should bear responsibility of not only testing it, but if his suggestion breaks something as well, no? Are you suggesting he pass off responsibility to someone else for his suggestion?
    – Tombo
    Commented Jan 10, 2019 at 19:57
  • @Dan, in last comment I should have said, "Otherwise he would be passing off responsibility to someone else for his suggestion." Won't let me edit comments after 5 minutes.
    – Tombo
    Commented Jan 10, 2019 at 20:04
  • This is still pretty awkward for the senior dev, which translates to an awkward relationship between senior dev and junior dev. If they already have a good working relationship this could be ok. Otherwise it could be gasoline on the fire.
    – bob
    Commented May 7, 2021 at 19:20

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .