Provide true clock-modulus operation for arbitrary sign values

Question

I wrote this template to do as stated in the title; did I miss anything?

template<typename T> T clockmod(T v, T m) //todo: test T is numeric
{
 typedef long double D;
 T t=(m-m); //zero default
 D dv=std::fabs((D)v);
 D dm=std::fabs((D)m);
 if ((dv>0.0)&&(dm>0.0))
 {
 int id=int(dv/dm);
 D d=(dm*id);
 D dr=(dv-d);
 if (v>0)
 {
 if (m>0) t=(T)dr;
 else if (m<0) t=(T)(dr-dm);
 }
 else if (v<0)
 {
 if (m>0) t=(T)(dm-dr);
 else if (m<0) t=(T)(-dr);
 }
 }
 return t;
}

Where:
v / m may be any numeric type;
m (the modulus base) always range from 0 to m, m<0 or m>0;
the sign of v indicates the 'direction of counting' round the clock, positive increments from 0, negative decrements from m

• Heslacher: Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers .

Answer 1

Style comments:

• Generic typename T implies you do not know or care what kind of type it is or which properties it has. Why not name it Numeric or something in that direction?
• You should really use better names for variables. At first glance, no one has any idea what which variable is intended to do.
• Even a single line if deserves braces. When omitting these and later adding to the code, programmers often forget to add them and introduce bugs.
• typedef is a relic of C compatibility. Consider the using keyword instead.
• Do not just arbitrary rename a type especially into something like D. You have an IDE it can do completion for you, use it. (And yes, even vim and emacs do completion, no excuses unless you write your code in notepad.)
• Comment on your intentions and assumptions of the pieces of your code.

Solution issues:

the sign of v indicates the 'direction of counting' round the clock, positive increments from 0, negative decrements from m

• You are probably looking for this, meaning you should declare the template function and then offer specific implementations for the different possible instances of T.
• Is a cast to long double really required? If T is a numeric type, all required operations should be defined for T. There is no reason to fall back to std::fabs().

• If you want to do it by hand, I would suggest something in the way of

  bool clockwise = (v >= 0);
  T absolute_v;
  if(!clockwise){ // !clockwise implies v < 0
   absolute_v = -v; // we want the abs(v)
  } else {
   absolute_v = v;
  }
  

• Cody Gray: I disagree that typedef "is a relic of C compatibility". It isn't a relic at all: it is a full part of the C++ language standard and there's nothing wrong with using it. using is a new construct in C++11, and only has an advantage over typedef in certain cases involving templates, where it allows shorter code to be written. In all other circumstances, they are precisely equivalent, and I don't think there is anything wrong with using typedef. (Except, of course, as you point out in the next bullet, it shouldn't be used here at all!)
• slashmais: good advise, thx. I changed my code based on your suggestions and added it as an answer. The this-link you gave concerns the %-operator which only returns a remainder, but I took cognizance that floating-points operations are problematic.
• fer-rum: @CodyGray As a rule of thumb, I would discourage type redefinitions of any kind. Usually when a type gets far too unwieldy to be completed by a proper IDE, one might think about introducing intermediate classes. Nowadays, one should - if the task permits - write at least C++11 if not '14.

Answer 2

Based on fer-rum's answer:

template<typename NumType> NumType clockmod(NumType value, NumType base)
{
 NumType result=0; //zero default
 //NumType = {integral/floating point} - calc absolute values manually
 NumType abs_value=(value<0)?(-value):(value>0)?value:0;
 NumType abs_base=(base<0)?(-base):(base>0)?base:0;
 bool bIncrementing=(value>0);
 bool bPositiveBase=(base>0);
 if ((abs_value>0)&&(abs_base>0))
 {
 int nbases=int(abs_value/abs_base);
 NumType remainder=(abs_value-(abs_base*nbases));
 if (bIncrementing)
 {
 if (bPositiveBase) result=remainder;
 else result=(remainder-abs_base);
 }
 else
 {
 if (bPositiveBase) result=(abs_base-remainder);
 else result=(-remainder);
 }
 }
 return result;
}

Must say it looks better, less cryptic (blame my C-heritage). Also self-documenting code now.

---
Second try:

template<typename NumType> NumType clockmod(NumType value, NumType base)
{
 NumType result=0; //zero default
 //NumType = {integral/floating point} - calc absolute values manually
 NumType abs_value = ((value < 0)? (-value) : value);
 NumType abs_base = ((base < 0)? (-base) : base);
 bool bIncrementing = (value > 0);
 bool bPositiveBase = (base > 0);
 if ((abs_value > 0) && (abs_base > 0))
 {
 int nbases = int(abs_value / abs_base);
 NumType remainder = (abs_value - (abs_base * nbases));
 if (bIncrementing)
 {
 if (bPositiveBase) { result = remainder; }
 else { result = (remainder - abs_base); }
 }
 else
 {
 if (bPositiveBase) { result = (abs_base - remainder); }
 else { result = (-remainder); }
 }
 result = (result==base)? 0 : result; //base is 0-point
 }
 return result;
}

(Also slipped in a small fix :)
As for readable maintainable code I would be happy to work on code written like the above. This was a useful exercise.
Just for comparison's sake, a 'compact' version of the above - it speaks for itself:

template<typename N> N cmod(N v, N b)
{
 N r=0, d, av=((v<0)?(-v):(v>0)?v:0), ab=((b<0)?(-b):(b>0)?b:0);
 if ((av>0)&&(ab>0))
 {
 d=(av-(ab*int(av/ab)));
 r=(v>0)?((b>0)?(d):(d-ab)):((b>0)?(ab-d):(-d));
 r=(r==b)?0:r;
 }
 return r;
}

• fer-rum: Now, if you would just use braces on the single-statement ifs and maybe introduce some spaces, this would be fine code. Note about the ? operator: Some people do not mind it, but I personally would discourage using it in favour of if-else constructs. This however is a pure personal opinion and there will be someone saying it is fine right around the corner...
• SparK: NumType abs_base=(base<0)?(-base):(base>0)?base:0; can't this be simply NumType abs_base=(base<0)?(-base):base; zero is zero... or better yet, include <math.h> and abs_base = sqrt(pow(base, 2));
• Cody Gray: I agree with the first part of your comment, @SparK, but not the second. If this is to be a generic (template) function, you shouldn't call sqrt or pow functions because they aren't implemented for integer values. Integers will have to be converted to floating-point values, and then converted back, which will be extremely slow. (And pow(x, 2) is an anti-pattern in any case—just write x * x.)
• Cody Gray: Someone who thinks the conditional operator is fine reporting for duty. :-) But I do recognize that it can be obfuscated. You absolutely must use whitespace around the operators, which your code doesn't do. If you have especially complicated expressions used with the ternary operator, it can even help to break them into two lines. That makes a ternary operator equally as readable, if not more so, than an if/else block. As for why you'd want to do this, well, you often need it in a member initialization list or anywhere else you can't write a block. Also, some compilers optimize these better.
• slashmais: @fer-rum: thank you, it's my first time here and I found the advise pertinent and useful, also for private code (that I sometimes look at and wonder wtf is going on there - see the compact example). Is there some 'standards' or 'best-practices' document available?