I used to phrase what I thought were helpful tips in this way. I thought that by phrasing it as a question, I was showing that I wasn't making assumptions about the other person's level of skill, and was leaving room for them to know better than me. What is the better way to phrase it? "You should put this in a function." - I seem presumptuous, arrogant, patronizing. "Why not put this in a function?" - Implies "the other person is somehow strange for not having arrived at the same conclusion." "If this were in a function, the code would be easier to read." - Implying that the code is hard to read. Giving advice to colleagues who lack skill is something that I have no idea how to do in a sensitive way, and no one ever tells me how they'd like me to do it. This article points out a serious problem in tech culture, but offers no solution while at the same time implying that the solution is obvious. As I write this, I realize the irony in the article: The sentence commits the very sin it criticizes. How meta! I like it for the empathy it elicits, but I churn inside with the feeling of being oh so close and yet so far. How do I phrase criticism of code as a helpful tip? I can read all the MVC articles in the world, but they don't do me a lick of good if they don't have concrete suggestions for how I should change my behavior aside from "stop doing X". I readily follow "stop doing X" advice so long as X is not essential to the job. But I can't just stop helping people improve their code.“Huh… why didn’t you just turn this into a function?” Comments that could have been phrased as helpful tips are instead often delivered in ways that imply the other person is somehow strange for not having arrived at the same conclusion.
“Huh… why didn’t you just turn this into a function?” Comments that could have been phrased as helpful tips are instead often delivered in ways that imply the other person is somehow strange for not having arrived at the same conclusion.
(Sorry, I have not read the article yet, but I DO perform a lot of code reviews)
"This logic is repeated here and here, so if it's put into a function, then if we ever need to change that, we change it in one place."
i.e. tell them why it should be in a function.
I like this. It's difficult for me to generalize from examples like this, though. I am terrible at giving code reviews to people who aren't good at receiving criticism. I hesitate before making statements, which gives the impression that I think something's bad with the code, which is TRUE, but I don't know how to soften the blow. How do you handle people who take criticism of their code as an attack on their identity?
It's a hard problem. I had an instructor at Uni that gave some advice in this regard that has stuck with me (which probably won't help you much, but there you go). People don't like their code criticised because they see it as an extension of themselves, so, if you can, try instead to think of your code more as your child - it needs to be strong to survive when you're not around; and criticism helps make your child stronger. Separate it from your identity and some of the sting goes away. This really only helps in taking criticism, though.
I find it helps (a little) to do code reviews through tools instead of face-to-face. Smart Bear software have a pretty good (and free) code review tool. And try to only criticise things that matter; resist the urge to nit-pick as much as possible. Have a coding standard, to help justify your critiques and make it less personal.
Giving helpful criticism is difficult, sometimes I think it has to do more about tone and the relationship between the people involved. Having a relationship of respect, knowing not to take criticisms of your work personally, and being able to make mistakes without being made to feel humiliated are all important, IMO. If you have all that, "Huh... why didn't you just turn this into a function?" might be an acceptable comment (personally I'd probably phrase it "This could probably be a function.")
It's great to help people improve their code, and to think about how advice can be delivered respectfully. It's subtle, but "just" is loaded. I'm sure you've written code under a crazy deadline that could have been better. I can imagine either asking "Is there a reason this isn't in a function?" or saying, "Hm. That could be in a function." I think the difference is that in those cases, you're attempting to understand where the person you're helping is coming from (maybe she is in a hurry and hasn't refactored, maybe she has some cool new grand plan for how the code should work, maybe she didn't notice the code could be reused, or maybe a function actually didn't occur to her), instead of assuming she doesn't know or doesn't care. It is subtle, I agree.
This is your best option. Code is usually hard to read, and anything you can do to alleviate that is good! If you focus on what you think would be a positive outcome of your advice (without phrasing it to emphasize the current negatives, i.e. that their code is unreadable), then your advice will be less embarrassing to hear and more likely to be followed."If this were in a function, the code would be easier to read."
I've worked with dogmatists and I ignored their advice for a long time because I didn't see the value of it in the code they produced. Then I read the original source material of their dogma, saw that it was good, and saw how wrongly they were applying it. Then I became a better programmer and quit. THE MORAL OF THE STORY IS: Make sure I understand an idea before I dismiss it because someone I don't like misunderstands the idea themselves. This is a variant of strawman fallacy.