Re: Shift elements of an array

On Monday, October 7, 2013 8:03:31 AM UTC+2, Rosario1903 wrote:


Usually when people post code in comp.lang.c++, at least as of old they hav=
e expected a review. Else why post it? So, review.

First, I illustrate the main point, that the code is needlessly complicated=
 and verbose. The complexity also means that the code's brittle (will easil=
y break in maintainance). Here's shorter and simpler code that produces ess=
entially the same output as your program ("essentially": the lead text has =
been clarified):

[code]
#include <iostream>
#include <string>
using namespace std;

void shift_left( string& s, int n = 1 )
{ s = s.erase( 0, n ) + string( n, ' ' ); }

auto main() -> int
{
    string s = "this and that";
    cout << "at start: '" << s << "'\n";

    shift_left( s, 3 );
    cout << "after shift 3: '" << s << "'\n";

    for( int i = 1; i <= 12; ++i )
    {
        shift_left( s );
        cout << "after shift 1: '" << s << "'\n";
    }
}
[/code]


Macros do not respect scopes, so they can easily lead to unintended text su=
bstitutions.

Therefore, to the degree possible, avoid macros.

And where you do use macros, adopt the (now) common convention of ALL UPPER=
CASE names for macros, which reduces the probability of unintended text sub=
stitution.

The u32 definition does not necessarily define a 32-bit type name.

If you really want a 32-bit thing, use the <stdint.h> header.


Instead of C level malloc and free, use C++ new and delete.

And instead of C++ new and delete, preferentially use standard library cont=
ainers such as std::vector, which deal automatically with deallocation.

One reason is that otherwise you will likely run afoul of the "rule of thre=
e" (or in C++11 sometimes now referred to as the "rule of five").

Essentially that means getting Undefined Behavior.


Here with a std::string or std::vector you could just say

    myarray(): arr( 15, ' ' ) {}

However the effect is /slightly/ different.

Your code produces an array of 15 items each of INDETERMINATE VALUE, while =
the suggested replacement produces an array of 15 items each with the space=
 character code (which in ASCII is 32).


Since this generalizes the default constructor, you could code both up as t=
he same constructor with a defaulted argument, like this:

    myarray( u32 c = 15 )

However, in order to avoid problems with implicit promotions, which can cau=
se unintended wrap-around with unsigned types, preferentially use a signed =
type, and also, to enhance readability preferentially use descriptive names=
:

    myarray( int size = 15 )
 


Here better use an "assert", from <assert.h>.


As before, better use C++ new and delete, and wrt. those operators, prefere=
ntially instead use standard library containers.


As before, use "assert".


Regarding the member initializations, in some cases one HAS TO use memory i=
nitializers, the ":" notation. And that's generally most efficient also. So=
 as a general rule, prefer ":" to assignments in a constructor body.


Here the "char*" prevents the code from compiling with a conforming C++ com=
piler.

That's because the C implicit conversion from literal to non-const char* wa=
s deprecated in C++98 and REMOVED in C++11 (the current standard).

AFAIK most compilers still support that conversion, but they will probably =
not continue to support it for long.

So, use "char const*".


Generally the specific indentation rules don't matter as long as they're ap=
plied consistently.

However, the above indentation rules, which result in different indent size=
s, is not very clear.

To some degree it defeats the purpose of indentation.


The lack of "const" prevents passing a "const" myarray to <<.

Do like this:

  friend ostream& operator<<( ostream&, myarray const& ma )


Better establish a guaranteed s != 0 in every constructor.

And don't change that property in any member function.

That's then called a CLASS INVARIANT, and can then simply be assumed everyw=
here without further checking.


Ken Thompson (I think it was) was once asked what he'd do different if he w=
ere to design Unix again.

Answer: "I'd spell creat with an E".

Just avoid silly shortenings like "shiftd". Spell it out, like "shifted".

There is another possible improvement here, because a member function that =
updates this object's value with a copy of a modification of another object=
's value, is quite unusual and counter-intuitive.

Instead just provide a pure modifier member function, or a pure value produ=
cer member function.


Always returning the same numeric value indicates that a numeric function r=
esult is not really meaningful.

Perhaps this member function should have been declared "void".


Generally, when a class has at least one constructor, then it's a good idea=
 to make data members "private" or at least "protected", so as to avoid oth=
er code inadvertently changing those data members directly and e.g. invalid=
ating the class invariant.


The "void" here is good practice in C, where it specifies that this functio=
n has no arguments, as opposed to accepting any number of arguments.

In C++ it's accepted just for C compatibility, but has no purpose: in C++ t=
he signature "int main()" already says that there are no arguments.

So, it's a C-ism, which is best avoided -- because the two languages are fu=
ndamentally different, and should best not be conflated with each other.


In C++ better declare variables as near their first use as possible.

In this case, write e.g. "for( u32 i = 0, ...".

However, as mentioned, it's even better to write just "for( int i = 0, ..=
..".


In both C++ and C it's now unnecessary to return 0 from main, as that's the=
 default.

It has always been the default in C++.

However, many programmers still prefer to indicate the "main" return value =
explicitly. For that purpose it can be a good idea to use the symbolic valu=
es EXIT_SUCCESS and EXIT_FAILURE from <stdlib.h>. In this case, EXIT_SUCCES=
S, which is what 0 means in this context.


As a general comment, since myarray does not take charge of copying -- it=
 does not declare any copy constructor or assignment operator -- any use =
of it that inadvertently invokes copying, will in general cause the same me=
mory to be deallocated twice (via the free call in the destructor), invokin=
g Undefined Behavior, such as e.g. a crash... :-(

In C++03 this is known as the "rule of three":

namely, if you need a destructor, a copy constructor or an assignment opera=
tor, then you most probably need ALL THREE.


Again, note that the much shorter and simpler code shown at the start, prod=
uces essentially the same output.

Simpler and shorter = safer.

Cheers & hth.,

- Alf