Mixing
What to do when the developer whose code I review becomes defensive?
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
0
down vote
favorite
I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.
Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.
At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.
Should I stop doing code reviews? How to approach this situation in the best way?
Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?
software-industry work-environment team software-development teamwork
edited 11 mins ago
Anne Daunted
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
 |Â
show 1 more comment
up vote
0
down vote
favorite
I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.
Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.
At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.
Should I stop doing code reviews? How to approach this situation in the best way?
Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?
software-industry work-environment team software-development teamwork
edited 11 mins ago
Anne Daunted
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago
 |Â
show 1 more comment
up vote
0
down vote
favorite
up vote
0
down vote
favorite
I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.
Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.
At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.
Should I stop doing code reviews? How to approach this situation in the best way?
Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?
software-industry work-environment team software-development teamwork
edited 11 mins ago
Anne Daunted
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
I am in the following situation. Small team within a large unit with many teams. There is an official code review process, but to a lot of developers it is more of a formality so it is a rare thing to see serious discussions or review.
Within my team I encountered a developer who was not following the routine and was committing straight to the develop branch. I opened the topic of code reviews, then we started doing pull requests. My pull requests were automaticaly approved. It was though obvious that he was not reading them, just pressing the button.
At the same time it appears he is taking it a bit personal when I am doing the reviews. I usually do quite thorough code reviews to be honest. I take it seriously. I have been doing code reviews to other developers in the company I work for it is just this one case where the developer appears to be extremely defensive. The problem is that we work very close.
Should I stop doing code reviews? How to approach this situation in the best way?
Just one fresh example to get the point. The developer writes a class that looks like something in between Factory and Container, then names it SomethingBuilder where the Something part is not related to the finally built object. I of course comment on this and let's say he takes it personally. What am I supposed to do here? It appears the Tech Lead of the team does not care. He defends him a lot actually. He says everyone has their own style. I am not sure what kind of style it is to name Factory a Builder, but nevermind. Is avoidance the best strategy here, especially when the Tech Lead shows compassion to the guy?
software-industry work-environment team software-development teamwork
software-industry work-environment team software-development teamwork
edited 11 mins ago
Anne Daunted
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 11 mins ago
Anne Daunted
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
edited 11 mins ago
Anne Daunted
6961616
edited 11 mins ago
Anne Daunted
6961616
edited 11 mins ago
Anne Daunted
6961616
6961616
asked 1 hour ago
Pesho
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
asked 1 hour ago
Pesho
73
asked 1 hour ago
Pesho
73
73
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
New contributor
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
Pesho is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago
 |Â
show 1 more comment
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago
 |Â
show 1 more comment
1 Answer
1
active
oldest
votes
up vote
0
down vote
I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.
Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.
If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.
As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.
It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?
If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.
A Discussion on Naming
Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.
Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.
Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:
- describe the output
- describe the business logic contained within
Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:
saveDataInAthreadSafeAndDataConsistentManner()...
isn't useful. But
saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()
well, that's a useful method name.
But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.
Another Thought
Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.
answered 1 hour ago
bharal
12.5k22657
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
 |Â
show 6 more comments
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
0
down vote
I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.
Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.
If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.
As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.
It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?
If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.
A Discussion on Naming
Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.
Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.
Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:
- describe the output
- describe the business logic contained within
Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:
saveDataInAthreadSafeAndDataConsistentManner()...
isn't useful. But
saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()
well, that's a useful method name.
But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.
Another Thought
Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.
answered 1 hour ago
bharal
12.5k22657
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
 |Â
show 6 more comments
up vote
0
down vote
I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.
Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.
If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.
As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.
It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?
If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.
A Discussion on Naming
Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.
Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.
Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:
- describe the output
- describe the business logic contained within
Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:
saveDataInAthreadSafeAndDataConsistentManner()...
isn't useful. But
saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()
well, that's a useful method name.
But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.
Another Thought
Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.
answered 1 hour ago
bharal
12.5k22657
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
 |Â
show 6 more comments
up vote
0
down vote
up vote
0
down vote
I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.
Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.
If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.
As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.
It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?
If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.
A Discussion on Naming
Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.
Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.
Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:
- describe the output
- describe the business logic contained within
Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:
saveDataInAthreadSafeAndDataConsistentManner()...
isn't useful. But
saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()
well, that's a useful method name.
But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.
Another Thought
Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.
answered 1 hour ago
bharal
12.5k22657
I'm not sure there is much to do here. If the most egregious example you have is the naming of a class - and it's not even that badly named, I'm honestly uncertain of the difference between "builder" and "factory" - then I think you might want to reconsider what you're reviewing in a code review.
Look, a review doesn't always have to have a problem. If the developer is matching team naming conventions, whatever they are, then you don't need to comment on the names.
If you catch, say, a memory leak, or some weirdly ineffective code then yes, note that. But don't fall into the trap of "when asked to comment on something i have to comment on it in some way". You don't. You're in industry, 75% is a commendable effort, hell, 35% is a pass most times.
As long as the things work and doesn't malfunction and it's done in time, well, what more do you want? You're not being paid by lines of beautiful code, you're not being paid by lines of code at all - you're being paid by practical output.
It sounds like you're adding busy work to the team, be careful. Look at it this way - if you were running a company, would you want beautiful code that didn't do anything, or rubbish code that barely scaled - but you could sell?
If you think there's anyone running a company alive that wants the first one, you might be right - but they're not running that company for very long.
A Discussion on Naming
Naming conventions are stupid. Words change their meaning over time, and trying to cement the name to the function is pointless when the function is right there and can be read.
Anyone can read the code to understand what it does. But everyone will have a different idea of what words mean - so insisting the words match the meaning for your understanding of the words is a broken premise, unless you're the only person who will ever read the code, which you are not.
Worse, you want the names to match the inner workings when, really, names should do one of (or better, both of) two things:
- describe the output
- describe the business logic contained within
Describing the inner workings is mostly pointless, because, again, the code can be read. Capturing the business logic is much, much more important. To whit:
saveDataInAthreadSafeAndDataConsistentManner()...
isn't useful. But
saveAddressDetailsAsTheresAConcurrentProblemWhenSavedWithEverythingElse()
well, that's a useful method name.
But at the end of the day, working code >> well named code that doesn't work, and quickly produced working code >> slowly produced well named working code.
Another Thought
Don't ever look like you're the person in the team enforcing standards that slow the team down - that's not a good look. If your team lead agreed with you then you'd be ok to pursue this, but the TL doesn't, so now it looks like you're just fussing over naming conventions. I wouldn't do this.
answered 1 hour ago
bharal
12.5k22657
edited 1 hour ago
answered 1 hour ago
bharal
12.5k22657
answered 1 hour ago
bharal
12.5k22657
answered 1 hour ago
bharal
12.5k22657
12.5k22657
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
 |Â
show 6 more comments
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
What are you saying now ? That you don`t know what is the difference between Builder pattern and Factory pattern ? Or that in your opinion it is not a big deal to name a Factory pattern to be Builder pattern? Actualy it is not even a Factory I gave it as the closest thing to it but that is a different thing. In the same context is it ok if I name the Toilet to be pissuare and the pisuare to be toilet ? It would be interesting if someone is blind what will he do.
– Pesho
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
@Pesho I probably don't. To me they both make something, the underlying difference is just how they make that thing. But I wouldn't care how the thing is made, if I were a consumer of it I'd just happily use it. Enforcing the naming conventions as you are means that you want the name to clearly match the how. Sigh, i'll expand my answer...
– bharal
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I want the name to match at least "What". It is not building what it is saying that it is building.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
I agree with what you have written in the "Discussion on Naming". And I repeat again the class is not doing what it says it is doing. If we puit aside the Builder part, it is saying that it is building one object when in reality it is building another.
– Pesho
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
@Pesho that's a different issue - i think you should drop the "builder v factory" issue and ask your TL again about the "something" bit. But if your TL is against it, then let it slide. It's not going to win you points, or a promotion.
– bharal
1 hour ago
 |Â
show 6 more comments
Pesho is a new contributor. Be nice, and check out our Code of Conduct.
Pesho is a new contributor. Be nice, and check out our Code of Conduct.
Pesho is a new contributor. Be nice, and check out our Code of Conduct.
Pesho is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fworkplace.stackexchange.com%2fquestions%2f121672%2fwhat-to-do-when-the-developer-whose-code-i-review-becomes-defensive%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
what do you mean "a bit personal"? what do you mean "quite thorough" and "quite seriously", can you give an example of what you're rejecting or commenting on? what does "work very close" mean? physically? do you have a team lead in the team you can raise this to?
– bharal
1 hour ago
When I say throughrough, I mean that I am realy spending time to find the problems in the code and point out its weak points. I usualy avoid using the word "you" but reference the code itself.
– Pesho
1 hour ago
I mean I don`t do the reviews just formaly.
– Pesho
1 hour ago
are you just commenting on the class naming conventions? I can see how that would irritate someone - some developers use class names as an outlet for creativity. I think i once named a pattern matching class as "DeviceHarmoniser". If the worst example you can come up with in a code review is the naming of a class then I think it's safe to let that slide.
– bharal
1 hour ago
@bharal It is a very confusing class actualy. When you read it you expect a Builder that builds particular object. Then you realize it does not have the classic Builder api. Then you realize it is not actualy building what it says, and then you realize it does hell of a lot more than what it actualy Builds. It took me 30 min to understand the logic and I consider myself very fast code reader.
– Pesho
1 hour ago