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 have 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 easily break in maintainance). Here's shorter and simpler code that produces essentially 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]
> #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 UPPERCASE names for macros, which reduces the probability of unintended text substitution.
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 containers such as std::vector, which deal automatically with deallocation.
One reason is that otherwise you will likely run afoul of the "rule of three" (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, while the suggested replacement produces an array of 15 items each with the space 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 cause unintended wrap-around with unsigned types, preferentially use a signed type, and also, to enhance readability preferentially use descriptive names:
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, preferentially 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++ compiler.
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 probably 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 sizes, 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 everywhere 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 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 producer 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 idea to make data members "private" or at least "protected", so as to avoid other code inadvertently changing those data members directly and e.g. invalidating the class invariant.
> };
>
> int main(void)
The "void" here is good practice in C, where it specifies that this function 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 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 values EXIT_SUCCESS and EXIT_FAILURE from <stdlib.h>. In this case, EXIT_SUCCESS, 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 memory to be deallocated twice (via the free call in the destructor), invoking 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 operator, 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, produces essentially the same output.
Simpler and shorter = safer.