Style Case Studies: Index Tables - Improving Style
(Page 4 of 6 )
b) Stylistic improvements that would improve code clarity, reusability, and maintainability.
Beyond the mechanical errors, there were several things I personally would have done differently in the code example. First, a couple of comments about the helper struct:
template <class RAIter>
struct sort_idxtbl_pair
{
RAIter it;
int i;
bool operator<( const sort_idxtbl_pair& s )
{ return (*it) < (*(s.it)); }
void set( const RAIter& _it, int _i ) { it=_it; i=_i; }
sort_idxtbl_pair() {}
};
5. Be const correct. In particular, sort_idxtbl_pair::operator< doesn’t modify *this, so it ought to be declared as a const member function.
Guideline: Practice const correctness.
6. Remove redundant code. The program explicitly writes class sort_idxtbl_pair’s default constructor even though it’s no different from the implicitly generated version. There doesn’t seem to be much point to this. Also, as long as sort_idxbl_pair is a struct with public data, having a distinct set operation adds a little syntactic sugar but because it’s called in only one place the minor extra complexity doesn’t gain much.
Guideline: Avoid code duplication and redundancy.
Next, we come to the core function, sort_idxtbl:
template <class RAIter>
void sort_idxtbl( RAIter first, RAIter last, int* pidxtbl )
{
int iDst = last-first;
typedef std::vector< sort_idxtbl_pair<RAIter> > V;
V v( iDst );
int i=0;
RAIter it = first;
V::iterator vit = v.begin();
for( i=0; it<last; it++, vit++, i++ )
(*vit).set(it,i);
std::sort(v.begin(), v.end());
int *pi = pidxtbl;
vit = v.begin();
for( ; vit<v.end(); pi++, vit++ )
*pi = (*vit).i;
}
7. Choose meaningful and appropriate names. In this case, sort_idxtbl is a misleading name because the function doesn’t sort an index table… it creates one! On the other hand, the code gets good marks for using the template parameter name RAIter to indicate a random-access iterator; that’s what’s required in this version of the code, so naming the parameter to indicate this is a good reminder.
Guideline: Choose clear and meaningful names.
8. Be consistent. In sort_idxtbl, sometimes variables are initialized (or set) in for loop initialization statements, and sometimes they aren’t. This just makes things harder to read, at least for me. Your mileage may vary on this one.
9. Remove gratuitous complexity. This function adores gratuitous local variables! It contains three examples. First, the variable iDst is initialized to last-first and then used only once; why not just write last-first where it’s used and get rid of clutter? Second, the vector iterator vit is created where a subscript was already available and could have been used just as well, and the code would have been clearer. Third, the local variable it gets initialized to the value of a function parameter, after which the function parameter is never used; my personal preference in that case is just to use the function parameter (even if you change its value—that’s okay!) instead of introducing another name.
10. Reuse Part 1: Reuse more of the standard library. Now, the original program gets good marks for reusing std::sort—that’s good. But why handcraft that final loop to perform a copy when std::copy does the same thing? Why reinvent a special-purpose sort_idxtbl_pair class when the only thing it has that std::pair doesn’t is a comparison function? Besides being easier, reuse also makes our own code more readable. Humble thyself and reuse!
 This chapter is from Exceptional C++ Style, by Herb Sutter (ISBN 0201760428, copyright 2005. All rights reserved. It is reprinted with permission from Addison-Wesley Professional). Check it out at your favorite bookstore today.
Buy this book now. |
Next: Reuse and Preincrement >>
More Code Examples Articles
More By Addison-Wesley/Prentice Hall PTR
