4 ms·
Keep in mind that these are undergrad projects, so a few hundred LOC tops. But you'll see something like: bool noMoreElves() { return elfCount == 0; }
by BruceIV 14y ago
Keep in mind that these are undergrad projects, so a few hundred LOC tops. But you'll see something like:
bool noMoreElves() {
return elfCount == 0;
}
void fireAllReindeer() {
for (int i = 0; i < reindeerCount; ++i) {
delete reindeerArray[i];
}
delete reindeerArray;
}
void checkCloseWorkshop() {
if ( noMoreElves() ) {
fireAllReindeer();
}
}
the above is certainly readable, but takes forever to parse -- the below is, I would argue, just as clear, and much quicker to read (the point the OP made)
void checkCloseWorkshop() {
if ( elfCount == 0 ) {
// fire all reindeer
for (int i = 0; i < reindeerCount; ++i) {
delete reindeerArray[i];
}
delete reindeerArray;
}
}
the comment above the loop is possibly superfluous, but helpful
- skywalk 14y agoThat goes against many recommendations I've read about refactoring and writing cleaner code: "Good code invariably has small methods and small objects. I get lots of resistance to this idea, especially from experienced developers, but no one thing I do to systems provides as much help as breaking it into more pieces." – Kent Beck
- dalke 14y agoThe little research I've seen has not backed up that opinion. While that view is shared by many - especially those who come from a Smalltalk heritage - the studies I've read about do not show a clear association between small pieces and more readable/maintainable code, at least not for code blocks under 100 lines or so. I dislike reading Smalltalk influenced code with many small pieces. They tend to use instance state as a way to pass parameters, and that makes me uncomfortable - I have a leaning towards functional programming. I also distrust function names. A function like "noMoreElves" might actually be "no more elves!" and implemented as "elfCount = 100". I exaggerate a little, but an inline "elfCount == 0" is much easier to verify than having to jump elsewhere to see the code.
- ramblerman 14y agohttp://stackoverflow.com/questions/153234/how-deep-are-your-unit-tests/153565#153565 http://stackoverflow.com/questions/153234/how-deep-are-your-... Kent beck also gave this amazing (and suprising) answer on SO about "how far to go with your tests?" He is in my eyes, a true pragmatist and gives some sound advice. Uncle bob on the other hand seems to have drunk too much of his own koolaid. I saw him give a talk 2 years ago at a java conference which made him seem more like a Steven Balmeresque salestype, not an active developer.
- Noughmad 14y agoBoth of your examples are fine, as it is pretty clear what each method does. In such cases, the golden rule is "if you do it more than once, create a new function". I don't know what kind of things you program if you have 100-line functions with no code sharing.
- qu4z-2 14y agoThe main point is that when you realise you forgot to set reindeerCount = 0. If closing the workshop is the only situation in which you fire all the reindeer, it's probably fine. If you need to fire the reindeer in other situations, you now need to manually fix the code everywhere.
- BruceIV 14y agoThanks, good catch (my code probably would have been better if I had an actual use-case instead of a contrived example).
- d0m 14y agoAs far as I understand it, it's more of a "Bottom/Top" versus "Top/Bottom" thinking. Personally, looking at: void checkCloseWorkshop() { if ( noMoreElves() ) { fireAllReindeer(); } } is enough, I don't need to look more precisely. But, for someone who is more "Bottom/Top", the second version is a headache because they need to jump everywhere and keep 10 different files/functions open all the time. An anecdote of this, I once worked with a programmer who was "Bottom/Top" and really hated functions. (1500+ lines function was the way to go for him). He would then refactor my code like this: void checkCloseWorkshop() { bool noMoreElves() { return elfCount == 0; } void fireAllReindeer() { for (int i = 0; i < reindeerCount; ++i) { delete reindeerArray[i]; } delete reindeerArray; } if ( noMoreElves() ) { fireAllReindeer(); } } I.e. Pasting my functions into the other functions he needed.. rather than just calling mine or moving them in another file. Obviously, there was no tests or frameworks used by this company. This is a true example of "Bottom/Top" thinking. Last note, I understand your point but there's a way to mix both strategies by using high-level strategies. For instance, fireAllReindeer is deleting an array while noMoreElves is checking for emptiness. Good C++ programmers would use boost or the STD functors/SmartPointers rather then manually reinventing the wheel every time.