4 ms·
I don't love this refactor. I especially don't like how it generates names from other names -- e.g., given 'switch'/'submit', appends '_row', and then converts
by lincolnq 14y ago
I don't love this refactor. I especially don't like how it generates names from other names -- e.g., given 'switch'/'submit', appends '_row', and then converts it to CamelCase so you get 'SwitchRow'/'SubmitRow', then instantiates the class named that.
Here's why: When I am maintaining somebody else's code, even if I guess that this is going on, I rarely trust these kinds of name-generation tricks. When I need to change some code, I'm going to grep the codebase for 'SwitchRow' and add a new line in whatever conditional to instantiate my new class. When grep can't find any instances of SwitchRow's constructor being called, I become suspicious and it costs me more time, because I am making sure I properly understand what is actually going on.
- rosser 14y ago...I rarely trust these kinds of name-generation tricks. So you're probably not a big fan of Rails, then?
- hnewser1 14y agoand what does `trust` mean? You don't trust that correct object won't be given back to you?
- andreasvc 14y agoI suppose more like trust whether the resulting code will always be correct.
- diminish 14y agowhat about using a multiline YAML text to be loaded with automatic generation of objects from class name strings? (reminder!)
- potatolicious 14y agoI'm actually not a huge fan of implicit name-generation tricks, but I'm also not entirely opposed to them. The trick here I think is consistency - is name-generated type dispatch the standard across the entirety of the codebase? If it is, go wild. If it's not then I'd be much more wary of it. Consistency and building correct expectations for future maintainers is pretty important. Doing a smart trick in one place but failing to do it in other places where it makes sense IMO tends to create a bigger mess than doing it the dumb-but-safe way everywhere.
- SoftwareMaven 14y agoI so agree with you. There are few programming mistakes that consistency in use does ameliorate. On the other hand, no matter how "good" the code, if it's inconsistent, it's bad.
- Arelius 14y agoI disagree. Consistency is only valuable so long as it helps clarity and understanding. While that is most often the case, there are exceptions.
- justincormack 14y agoAgree. Names should not matter. You are going to have to document name construction. If your library users use IDEs you are going to have to get them to understand names.
- tantalor 14y agoAgreed. This "refactor" is actually a hack. It shouldn't pass code review.
- scrid 14y agoThis is especially true for those of us who do application support. When we are investigating application stack traces from production for a large application, it is awful when you discover that the developers decided to be clever with dynamic function names, making a large code base very difficult to support based on available logs. If you think you're being clever you're probably just being a dick.
- FuzzyDunlop 14y agoGiven this is ruby code, I think it's acceptable, and well-structured enough to be understandable. While the implementation itself might be a bit 'eugh' because of the meta-programming involved, there was enough thought put into code organisation to allow you to grep `RowType` to find the module that namespaces all these dynamically instantiated classes. Hell, if the code in the tree was arranged correctly, then `tree | grep row_type` would give you the folder where they all lived.