Re: [lmi] [lmi-commits] master 8862ffc 7/9: Improve const correctness

[Vadim Zeitlin]

On Wed, 12 Jun 2019 12:53:00 -0400 (EDT) Greg Chicares <address@hidden> wrote:

GC> branch: master
GC> commit 8862ffce33cd76fb09a5a5850d058a3807be9a17
GC> Author: Gregory W. Chicares <address@hidden>
GC> Commit: Gregory W. Chicares <address@hidden>
GC> 
GC>     Improve const correctness
GC>     
GC>     Replaced this idiom:
GC>       https://isocpp.org/wiki/faq/ctors#named-parameter-idiom
GC>     with a const variant that suffices for all real-world use cases.
GC> ---
GC>  dbindex.hpp    | 35 +++++++++++++++++++----------------
GC>  input_test.cpp | 28 +++++++++++++++++++---------
GC>  2 files changed, 38 insertions(+), 25 deletions(-)

 These const methods indeed seem to be sufficient, but for someone used to
the named parameter idiom use (such as me), they look quite confusing. I
think it would be nice to add a comment to the database_index class
explaining that we do _not_ use this idiom here. Alternatively, I'd
consider naming these functions new_with_foo() instead of just foo(), to
make it more clear that create a new object rather than modify the existing
one. This would make the code using them much longer, of course...

GC> @@ -96,18 +91,26 @@ class database_index
GC>      mcenum_uw_basis uw_basis () const {return 
static_cast<mcenum_uw_basis>(idx_[4]);}
GC>      mcenum_state    state    () const {return static_cast<mcenum_state   
>(idx_[5]);}
GC>  
GC> -    database_index& gender   (mcenum_gender   z) {idx_[0] = z; return 
*this;}
GC> -    database_index& uw_class (mcenum_class    z) {idx_[1] = z; return 
*this;}
GC> -    database_index& smoking  (mcenum_smoking  z) {idx_[2] = z; return 
*this;}
GC> -    database_index& issue_age(int             z) {check_issue_age(z);
GC> -                                                  idx_[3] = z; return 
*this;}
GC> -    database_index& uw_basis (mcenum_uw_basis z) {idx_[4] = z; return 
*this;}
GC> -    database_index& state    (mcenum_state    z) {idx_[5] = z; return 
*this;}
GC> +    database_index gender   (mcenum_gender   z) const {auto i = idx_; i[0] 
= z; return {i};}
GC> +    database_index uw_class (mcenum_class    z) const {auto i = idx_; i[1] 
= z; return {i};}
GC> +    database_index smoking  (mcenum_smoking  z) const {auto i = idx_; i[2] 
= z; return {i};}
GC> +    database_index issue_age(int             z) const {auto i = idx_; i[3] 
= z; return {i};}
GC> +    database_index uw_basis (mcenum_uw_basis z) const {auto i = idx_; i[4] 
= z; return {i};}
GC> +    database_index state    (mcenum_state    z) const {auto i = idx_; i[5] 
= z; return {i};}

 Another possible improvement could be to add symbolic names instead of
using the hard-coded 0..5 constants here. This again would make the code
slightly longer but I think it's worth it, "i[5]" really doesn't look very
self-documenting.

 Regards,
VZ

pgpL0Rz5arirb.pgp
Description: PGP signature