Re: Shift elements of an array
Your code is ignorant.
Remember if you use containers you should use types defined in them. So at =
least the function should be declared as
void shift_left( std::string& s, std::string::size_type n = 1 );
Moreover in this statement
s = s.erase( 0, n ) + string( n, ' ' );
you allocate a new string. It is simply a stupidy.
And the return type void in this case is the worst return type.
=D0=BF=D0=BE=D0=BD=D0=B5=D0=B4=D0=B5=D0=BB=D1=8C=D0=BD=D0=B8=D0=BA, 7 =D0=
=BE=D0=BA=D1=82=D1=8F=D0=B1=D1=80=D1=8F 2013 =D0=B3., 11:07:52 UTC+4 =
=D0=BF=D0=BE=D0=BB=D1=8C=D0=B7=D0=BE=D0=B2=D0=B0=D1=82=D0=B5=D0=BB=D1=8C al=
f.p.s...@gmail.com =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB:
On Monday, October 7, 2013 8:03:31 AM UTC+2, Rosario1903 wrote:
On Sun, 06 Oct 2013 18:59:06 +0200, Rosario1903 wrote:
the last version for this exercise.
Usually when people post code in comp.lang.c++, at least as of old they h=
ave expected a review. Else why post it? So, review.
First, I illustrate the main point, that the code is needlessly complicat=
ed and verbose. The complexity also means that the code's brittle (will eas=
ily break in maintainance). Here's shorter and simpler code that produces e=
ssentially the same output as your program ("essentially": the lead text ha=
s 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]
#include <iostream>
#include <cstdlib>
using namespace std;
#define u32 unsigned
#define u8 unsigned char
Macros do not respect scopes, so they can easily lead to unintended text =
substitutions.
Therefore, to the degree possible, avoid macros.
And where you do use macros, adopt the (now) common convention of ALL UPP=
ERCASE names for macros, which reduces the probability of unintended text s=
ubstitution.
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.
class myarray{
public:
~myarray(){free(arr);}
Instead of C level malloc and free, use C++ new and delete.
And instead of C++ new and delete, preferentially use standard library co=
ntainers such as std::vector, which deal automatically with deallocation.
One reason is that otherwise you will likely run afoul of the "rule of th=
ree" (or in C++11 sometimes now referred to as the "rule of five").
Essentially that means getting Undefined Behavior.
myarray(){arr=(u8*) malloc(15);
if(arr==0) exit(-1);
s=15;
}
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, whil=
e the suggested replacement produces an array of 15 items each with the spa=
ce character code (which in ASCII is 32).
myarray(u32 c)
Since this generalizes the default constructor, you could code both up as=
the same constructor with a defaulted argument, like this:
myarray( u32 c = 15 )
However, in order to avoid problems with implicit promotions, which can c=
ause unintended wrap-around with unsigned types, preferentially use a signe=
d type, and also, to enhance readability preferentially use descriptive nam=
es:
myarray( int size = 15 )
{if(c>0xFFFF) exit(-1);
Here better use an "assert", from <assert.h>.
arr=(u8*)malloc(c);
As before, better use C++ new and delete, and wrt. those operators, prefe=
rentially instead use standard library containers.
if(arr==0) exit(-1);
As before, use "assert".
s=c;
}
Regarding the member initializations, in some cases one HAS TO use memory=
initializers, the ":" notation. And that's generally most efficient also. =
So as a general rule, prefer ":" to assignments in a constructor body.
myarray(char* c)
Here the "char*" prevents the code from compiling with a conforming C++ c=
ompiler.
That's because the C implicit conversion from literal to non-const char* =
was deprecated in C++98 and REMOVED in C++11 (the current standard).
AFAIK most compilers still support that conversion, but they will probabl=
y not continue to support it for long.
So, use "char const*".
{u32 v, i;
if(c==0)
{v=1;}
else {v=strlen(c)+1;}
arr=(u8*)malloc(v);
if(arr==0) exit(-1);
if(v==1) *arr=0;
else {for(i=0; i<v ; ++i)
{arr[i]=c[i];
if(c[i]==0) break;
}
for(; i<v;++i)
arr[i]=0;
}
s=v;
}
Generally the specific indentation rules don't matter as long as they're =
applied consistently.
However, the above indentation rules, which result in different indent si=
zes, is not very clear.
To some degree it defeats the purpose of indentation.
friend ostream& operator<<(ostream& os, myarray& ma)
The lack of "const" prevents passing a "const" myarray to <<.
Do like this:
friend ostream& operator<<( ostream&, myarray const& ma )
{u32 i;
if(ma.s!=0)
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 ever=
ywhere without further checking.
{for(i=0; i<ma.s-1; ++i)
os<<ma.arr[i];
return os<<ma.arr[i];
}
else return os;
}
u32 shiftd(myarray& ma, u32 pos)
Ken Thompson (I think it was) was once asked what he'd do different if he=
were 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 tha=
t updates this object's value with a copy of a modification of another obje=
ct's value, is quite unusual and counter-intuitive.
Instead just provide a pure modifier member function, or a pure value pro=
ducer member function.
{u32 i, j;
if(s<ma.s)
{free(arr);
arr=(u8*)malloc(ma.s);
if(arr==0) exit(-1);
s=ma.s;
}
if(pos>=ma.s)
{for(i=0;i<s; ++i) arr[i]=0;}
else {/* 0 1 2 3 4 5 6 */
/* | shiftd 3*/
for(j=pos, i=0; j<ma.s; ++j,++i)
arr[i]=ma.arr[j];
for(;i<s;++i)
arr[i]=0;
}
return 1;
Always returning the same numeric value indicates that a numeric function=
result is not really meaningful.
Perhaps this member function should have been declared "void".
}
u32 s;
u8 *arr;
Generally, when a class has at least one constructor, then it's a good id=
ea to make data members "private" or at least "protected", so as to avoid o=
ther code inadvertently changing those data members directly and e.g. inval=
idating the class invariant.
};
int main(void)
The "void" here is good practice in C, where it specifies that this funct=
ion 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++=
the 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 =
fundamentally different, and should best not be conflated with each other.
{myarray v("this and that "), ma;
u32 i;
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, =
....".
cout<<"at start v=["<<v <<"]\n";
ma.shiftd(v, 3);
cout<<"at end ma=["<<ma<<"]\n";
for(i=0;i<12;++i)
{ma.shiftd(ma, 1);
cout<<"at end ma=["<<ma<<"]\n";
}
return 0;
In both C++ and C it's now unnecessary to return 0 from main, as that's t=
he default.
It has always been the default in C++.
However, many programmers still prefer to indicate the "main" return valu=
e explicitly. For that purpose it can be a good idea to use the symbolic va=
lues EXIT_SUCCESS and EXIT_FAILURE from <stdlib.h>. In this case, EXIT_SUCC=
ESS, 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 us=
e of it that inadvertently invokes copying, will in general cause the same =
memory to be deallocated twice (via the free call in the destructor), invok=
ing 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 ope=
rator, then you most probably need ALL THREE.
---------------
at start v=[this and that ]
at end ma=[s and that ]
at end ma=[ and that ]
at end ma=[and that ]
at end ma=[nd that ]
at end ma=[d that ]
at end ma=[ that ]
at end ma=[that ]
at end ma=[hat ]
at end ma=[at ]
at end ma=[t ]
at end ma=[ ]
at end ma=[ ]
at end ma=[ ]
Again, note that the much shorter and simpler code shown at the start, pr=
oduces essentially the same output.
Simpler and shorter = safer.
Cheers & hth.,
- Alf