Journal Articles
Browse in : |
All
> Journals
> CVu
> 115
(21)
|
Note: when you create a new publication type, the articles module will automatically use the templates user-display-[publicationtype].xt and user-summary-[publicationtype].xt. If those templates do not exist when you try to preview or display a new article, you'll get this warning :-) Please place your own templates in themes/yourtheme/modules/articles . The templates will get the extension .xt there.
Title: Comments on a Code Review
Author: Administrator
Date: 03 August 1999 13:15:32 +01:00 or Tue, 03 August 1999 13:15:32 +01:00
Summary:
Body:
responses from the Harpist
Thank you for another thought-provoking article - 'A Big Number Class'. I have taken the opportunity of adding a few comments and queries of my own, along with an amended version of your addTo function (kjm.txt). One point worth noting is that I rarely program 'seriously' in C++ at present (Delphi being more 'suited' to my current requirements) so some of my comments, from a C++ perspective, may appear rather naïve or just strange☺
I assume that you were going to replace the pre-processor constants; you seem to have got waylaid by the original author's interesting comments.
You are probably right.
My first reaction was to put them in the class as static consts (or enums for older compilers) but on closer inspection the constants all seem to relate to the implementation. I would therefore place them in an anonymous namespace in the implementation file, e.g.,
// aln.cpp namespace { const int MaxString = 1050; const int MaxAln = 32700; const int WordLength = 16; const int LongWordLength = 4; const int MaxNegWord = 0x8000; }
I agree, but I did point out that the code was written before namespace was introduced.
aln is indeed a fairly indifferent class name; whilst your suggestion of VeryLongInt seems fine, I would tend towards BigInt which is quite often used with such classes.
Yes, but it gets boring recycling other peoples names ☺
Again, the names of these functions are rather obscure. I do, however, feel that this highlights another defect, namely that the author only comments the obvious. In point of fact he appears to have misled you with his '// #define' comment. This is, I think, just a comment and not as one might suspect anything to do with conditional compilation. It's one of my pet peeves that programmers rarely comment header files. A simple one-line comment would help explain most well-written functions; even better if they came with a statement of pre- and post-conditions.
Yes, on re-reading his code I think he should have written something like:
// end of #defines
You queried why m_digits is unsigned int*; I would suspect that it points to a dynamically allocated array holding the representation. Whether it's desirable that the constructors should throw a bad_alloc is unclear (without sight of the class requirements) but it might be simpler to build the array into the class.
I would have made one of two other choices, either I would have used signed int (it has some advantages when handling intermediate results) or unsigned char (and stored a single digit per byte).
I'm uneasy about the use of static data members, esp. errnum. I'm almost certain that this will cause problems within a multi-threaded en-vironment. Using error codes instead of exceptions would require a separate errnum for each thread; doubtless thread libraries support this sort of thing, but that would render the solution (unnecessarily) implementation specific. I'm not particularly au fait with multi-threaded programming, but from my limited experience I would say that exceptions are safer and simpler than error codes here.
I think you are right.
On a first draft I'd go with the following non-explicit constructors:
aln(long = 0); aln(unsigned long); aln(string const&); aln(aln const&);
There may also be a case for a constructor with char const* on grounds of efficiency but I'd stick with string const& for the moment. I'd keep the non-virtual destructor as it appears aln is a concrete class.
I doubt if the class would be viable today without exceptions. Without them there is the danger of creating what are quaintly known as 'zombie objects', i.e., objects which are dead on arrival. Using error codes, client code needs to query each object after construction - aln could fail due to a lack of memory or more likely from incorrect input. Writing generic code with aln would, as a result, be problematical.
I would propose a simple exception class to replace error codes, something like aln_error perhaps:
struct aln_error { errnum n; aln_error(errnum err) {n = err;} };
Public functions setting errnum all need a throw(aln_error) specifier, all others should have no throw specifiers. Additionally, the constructors need a throw(bad_alloc) since they appear to be allocating dynamic memory.
I am less happy with your exception specifiers unless you are willing to do the work to capture and process all standard exceptions that maybe thrown from functions called by the implementation code.
I'm sure I'm not the only one who would like to hear more of providing debug support in a separate class. All too many publications, if they even consider the matter, seem to assume that assert is the only debug tool available.
I think Francis recently wrote a column on this subject for EXE Magazine.
This is the one area I have a couple of qualms with your solution:
-
The member function abs differs from the non-member in that it is a mutator. This opens up the possibility of a client using it by mistake. I've committed errors very similar to this and, of course, worse <sigh>.
-
Where possible, I try to restrict mutators to non-void member functions. This has improved my class designs; they seem cleaner and easier to reason about. I would, however, freely concede that such practice within the C-family is unusual. See B. Meyer's Object-Oriented Software Construction for a much fuller, if Eiffel-biased, account (Command-Query Separation principle).
FWIW my alternative would be the member function:
void changeSignTo(signflag sign) throw() {m_sign = sign;}
so abs becomes:
aln abs(aln const& item) throw() { aln temp(item); temp.changeSignTo(plus); return temp; }
I agree but for a slightly different reason (the return from my version should have been by value); making this a member function results in inelegant syntax for calling it. Of course it could be a public static function (I think I prefer that version as it removes the need for a changeSignTo member function.)
At the risk of appearing ignorant or at least more ignorant than most, I'd like to solicit your opinion on the use of friends in cases like this. First let me say I'm perfectly happy forwarding public virtuals to non-member functions instead of the unnecessary use of friends. However, I wonder where the gain is when the class (such as aln appears to be) is concrete? The alternative I considered was to use a private member function forwarded by a friend.
I would turn the question round. What is the advantage of using friend declarations when a class is concrete (does not have a virtual destructor)? It is simpler to use a single consistent method. If you later decide to change the design so that the class is no longer concrete it reduces change. Every time you make a decision you have a chance of getting it wrong.
-
The aln class is almost certainly a concrete class. As a result we have no need to forward a public virtual function to a non-member function such as would happen, for example, with i/o functions in a base class.
-
It minimises the public interface, removing potentially harmful or at least confusing duplication. I would consider a non-member function, such as abs, a part of the public interface - others, of course, might not.
-
The friend only accesses a private function (changeSignTo), rather than the more pathologically inclined friends that access data. I think this is a more benign form of friendship but maybe I'm wrong?
In general I do not disagree, but I still do not find it necessary. Whenever the intention is to return a modified copy by value I think that a public static member should be considered. Granted that the call will need to explicitly specify the class but that avoids unwanted and unexpected overloading.
Assumptions
-
length >= 0
-
rhs[length-1], lhs[length-1] exist
-
length of rhs >= length of lhs
-
all elements of rhs and lhs are >= 0
-
there will be no overflow
-
numeric_limits<signed char>::max() == 0x7F
typos
-
there is no return
-
|= used instead of &= to clear the carry bit
revised function
bool addTo(IntElem* lhs, IntElem const* rhs, signed char length) throw(){ // PURPOSE: add rep rhs[0..length-1] to lhs[0..length-1] // REQUIRE: length > 0 AND rhs[length-1], lhs[length-1] to exist AND // all elements of lhs and rhs to be >= 0 // PROMISE: RESULT = true on lhs[0..length-1] = old lhs[0..length-1] + rhs[0..length-1] // RESULT = false on overflow, lhs[0..length-1] undefined assert(length > 0); for (int i = 0; i < length - 1; ++i) { lhs[i] += rhs[i]; if (lhs[i] < 0) { lhs[i + 1]++; lhs[i] &= numeric_limits<IntElem>::max(); } // if } // for return (lhs[length - 1] += rhs[length - 1]) >= 0; } // addTo
Notes:
-
typedef unsigned char IntElem is used as we might want to change the representation to suit the underlying hardware.
-
numeric_limits<T>::max() is used instead of 0x7F
-
Simple tests (including overflow) have been conducted for 1, 2, 4, and 8 byte reps.
-
One slight drawback of using signed integers is if the user, as seems possible, wants multiples of 8-bits. This would require handling the most significant array element differently since its MSB (most significant bit) may not coincide with our carry bit.
-
I don't see an easy way of supporting aln objects with different array lengths, since this would be difficult to reconcile with a numeric_limits class. If we forget about numeric_limits we would then have difficulty using aln in place of a built-in integer type. As I'm short of time, I'll stick with a numeric_limits class, speaking of which...
The class needs an associated numeric_limits class defined, otherwise it will be difficult to use within generic code, e.g.,
Now spotting that puts you close to C++ expert.
template<class T> void doSomething(T& n) throw(Overflow) { if (n < numeric_limits<T>::max()) { n++; // further processing... } //end if else throw Overflow(); } // end of doSomething
A first attempt at such a class based on my underused copy of Stroustrup 3rd ed.
class numeric_limits<aln> { public static const bool is_specialized = true; static const int digits = 64; static const bool is_signed = true; static const bool is_integer = true; inline static aln min() throw() {return aln("-0xFFFFFFFFFFFFFFFF");} inline static aln max() throw() {return aln( "0xFFFFFFFFFFFFFFFF");} };
operator functions return by const value I seem to recall this is to prevent their use as lvalues, e.g., a + b = c should get caught be the compiler; but as a quiche-eating Delphi programmer I may well be wrong :)
You are correct.
Notes:
More fields may be available via dynamicdata ..