Questions containing the words:
What is the "best practice," or
What is the "right way," or
Which technique is "most popular"
are often received poorly here. Why is this?
How can I get a good answer to questions like this?
sigh I just need to vent. I'm doing code reviews. This developer doesn't seem to notice the difference between a class which is based on a database table (i.e. 1:1 between properties and columns) and one which doesn't relate to the database at all. He needed to pass some data, and went "Oh, this class is available there. Let me add some properties."
I really don't know if I want to send it back, or just let it go because it'd be more back-and-forth to get it corrected, and I don't think I can manage to explain to him the why.
Oh. And of the two entry points where it can be populated, one of them defaults to null if not supplied, and the other to empty spaces.
Are there different terms I can use to differentiate between "database-based classes" and UI-layer classes that don't correspond to the database? "Data Models" and "View Models"? Something clearer?
Also works with a result set from a query. Sorry to spoil your Sunday but what you're describing is a very bad idea. Unless the next thing you want is new fields showing up in the database for no better reason then they were in this class I'd do something about it. It might be time to schedule a "come to Jesus" lunch with him.
Indeed. I'd actually already written that. "This isn't *currently* a problem, but it will cause a lot of confusion down the road: Someone who isn't familiar with <the code base> will look at the class, see that field, and then wonder why it's not being retrieved correctly from the database. Then they'll wonder why the column isn't in the database and think that it's broken. Etc."
Oh this is currently a problem. It's just not causing a defect yet. This is a dirty trick. It's a lazy way to get something done at the expense of everyone who has to work with this. This kind of crap is why we have code reviews in the first place.
Some one used the kitchen to make a meal and left it in a mess. Just because the fly's haven't started buzzing yet doesn't mean there isn't a problem. Tell him his mother doesn't work here :)
A more professional way to put it, "The x class is used to model records in the y table of the database. This simple responsibility has been undermined by adding a, b, and c fields that are not meant to exist in the y table. These fields belong elsewhere."
It might be he was tapped out when he wrote it and that was the best he could think of. You might help him out by figuring out a good place to put them. But that's not your job. If you figure it out don't shove it down his throat in the review. Save it for if he asks. Give him a chance to ask. Otherwise it's his problem.
Let him come to you in confusion. I learned the hard way not to coach people in the write up of a code review. You're in a powerful position. You're standing between him and a promotion. Don't abuse this by getting chatty. If anything point out how you can tell this class is meant to model a record in the database so he won't make the same mistake again.
Once he's asking you it's much less confrontational. Don't be the reviewer that dictates everyone codes like them. Be the reviewer that people come to with their questions.
I see your point. I already tend to be the person that people come to with questions, but the point is well taken. There may be somewhere else that'd work even better, or he may find it himself (and learn!) but the worst case is just taking ~10 minutes to explain in person.
i.e. not so bad an outcome at the worst, and a good outcome at the best.
Questions containing the words:
What is the "best practice," or
What is the "right way," or
Which technique is "most popular"
are often received poorly here. Why is this?
How can I get a good answer to questions like this?
The Whiteboard
The Whiteboard