Re: Is this better? (was Re: Better way to program this?)

On Nov 4, 1:35 am, mike3 <mike4...@yahoo.com> wrote:


I may have this one totally wrong, but this looks
like a possible candidate for transform, where
what you do with the carry could be encapsulated
in your function object. I still can't figure
out exactly what the rules are concerning the
carry, despite reading your comments, but perhaps
I'm just I bad observer :-). If I were you
I would draw a little diagram explaining what
the algo should do e.g:

Sequence (or digit a):
5 2 [9 7] 1 4
Sequence (or digit b):
8 [1 4] 2 9 0
Should give result:
....
[] indicative of the ranges concerned.

With regards to the code presented, see below:


Why is DIGIT in caps? I presume its a macro. If it
is a macro, why use a macro, if not, then you should
not use caps.


Perhaps use a member function that returns the origin (2
versions, one returning const_iterator for constant RawInt
and the other returning non const for non constant ...:


Why not use std::min over here - I'll have to presume
MinInt does something special that differs from std::min.


I would declare tmp and carry here, not at the top.


Horizontal space makes things more readable, but opinions
may differ.


Above could look like this:

if( carry ){ carry = (tmp <= *bi); }
else { carry = (tmp < *bi); }

or even:
carry = carry ? (tmp <= *bi) : (tmp < *bi);

Rationale:
the boolean expression is already sufficient for the assignment


Ditto -> simply: carry = (tmp == 0) would do.


How about using std::fill here? Its perfect for zeroing.
In that case you would have to first carry carry over, then
fill the remainder. Regardless, the above is simple enough
to not require fill.

The idea that I've had (out of the box) was something in
the line of:

std::transform(
  s1_digit_pos, s1_end,
  s2_digit_pos,
  result_digit_pos,
  Op ); //Where Op is a binary operator.

<Op> could store the carry and implement it on the next item
in the sequence. <Op> could also contain state to indicate
whether an item should be added to the destination (or
result) sequence. transform makes a copy of <Op>, but
this does not matter as you could use something like
boost::ref to make the algo only use a reference to
Op. Eventually you have something like this (off
the top of my head). I'll use normal vectors for
simplicity.

#include <vector>
#include <functional>
#include <algorithm>
#include <boost/cref.hpp>

struct Op : std::binary_function<int, int, int>
{
  Op( int digMax )
  : digMax_( digMax ), carry_( 0 ){ }
  int operator()( int s1v, int s2v )
  {
    int result( s1v + s2v + carry_ );
    //I'm still not sure about when
    // carry has effect, but for example
    // lets assume when s1v + s2v > n
    carry_ = (s1v + s2v) > digMax_;
    return result;
  }
  const int digMax_;
  int carry_;
};

struct RawDigit
{
  void add(
    const std::vector<int>& s1,
    const std::vector<int> &s2 );
  std::vector<int> mySeq_;
};

void RawDigit::add(
  const std::vector<int>& s1, const std::vector<int> &s2 )
{
  Op op( 10 );
  //For simplicity, lets assume s1 and s2 has
  // same amount of items and this has more.
  std::transform( s1.begin(), s1.end(), s2.begin(),
    mySeq_.begin(), boost::ref( op ) );
  //Add the carry...
  mySeq.push_back( op.carry_ );
  //or perhaps... addCarry( op.carry_ );
}

All said, one needs to evaluate whether the algorithmic
approach is necessarily simpler (more clear). I feel
it does isolate the transformation better. Nevertheless,
as someone has often pointed out, there is nothing wrong
with non-algorithmic approach, as long as the intent is
clear (you should not even need comments IMO, but that's
another argument).

Regards,

Werner