11

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.

  • 17
    What actual, rather than theoretical, problems are these issues causing in your code? – Philip Kendall Mar 15 '16 at 6:53
  • 12
    @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 Mar 15 '16 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 Mar 15 '16 at 9:08
  • 7
    @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 Mar 15 '16 at 9:30
  • 3
    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 Mar 15 '16 at 21:31

11 Answers 11

35

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.

  • The question approach is very good! – jwsc Mar 15 '16 at 10:47
  • @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 Mar 15 '16 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 Jan 14 at 13:47
25

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.

  • 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 Mar 15 '16 at 13:20
  • Wow. Preach it, brotha. – MealyPotatoes Mar 16 '16 at 4:57
  • I am not a Java developer, but it seems quite possible that the senior developer here might have good reasons for doing things the way they were done. As (from in my own experience) not doing something the way you did it in school gives order-of-magnitude (or more) speed increase in a time-critical application... So why not start by simply asking why it was done this way? It's possible you might learn something. – jamesqf Mar 16 '16 at 19:44
  • 1
    @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 Jan 8 at 16:19
  • 1
    "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 Jan 8 at 16:33
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.

  • Good point, good strategy! This may lead to a stronger bond (rapport) indeed. Thank you! – pek Mar 15 '16 at 9:20
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.

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.

1

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.

0

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.

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.
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.....)

-1

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.

  • 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 Mar 15 '16 at 13:29
  • Someone has to lead. I'm speaking from personal experience. I didn't mean to confuse you. – HotBreakfast Mar 21 '16 at 15:10
  • I pity the fool that “harshes your mellow.” – Festus Martingale Jan 10 at 1:23
-1

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.

  • 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 Jan 10 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 Jan 10 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 Jan 10 at 20:04

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.