Re: What is the output of this program?
dnguyenk@yahoo.com wrote:
//...
string ToUpper(const string& s1)
{
string s2;
for (int i = 0; i < s1.size(); ++i)
if (isalpha(s1[i])) s2[i] = toupper(s1[i]);
return s2;
}
int main()
{
string v1[5] = {"This", "is", "a", "small", "quizz"};
string v2[5];
for (int i = 0; i < 5; ++i) v2[i] = ToUpper(v1[i]);
//..Print out v2. What is the output?
return 0;
}
A mistake made me stare at the screen for a while ...
Could you give some hints to avoid similar mistakes systematically?
Thanks.
Ng.
<SNIP>
There are at least two problems in the code:
1. The statement if (isalpha(s1[i])) s2[i] = toupper(s1[i]); does not
append any non-alphabetic characters to s2. This was the easy bit.
2. This one was a bit harder and it took me a while to see it after a
bit of testing (I am a beginner). String s2 is initialised to the empty
string in the
definition string s2; by std::string's default contructor, thus the
expression s2[i] is invalid for any i > 0. If you do as I did and
replace operator[] with the range-checked version s2.at(i) you will get
an index out of range exception being thrown. What was needed to fix it
is a line like
s2 += islower(s1[i]) ? toupper(s1[i]) : s1[i];
which appends each converted character to s2. The trouble with this of
course is that there are, at least potentially, O(n) allocations + 1
copy (the return
statement). I think we can get around this by calling
s2.reserve(s1.size()) so that we have only 1 allocation.
I'm still wondering why main() uses arrays rather than vectors.
Also, as has already been pointed out, you should not be using int as
the type of the index into the string. The type should be
std::string::size_type either that or use one of the string iterators.
What I ended up with is a loop like this:
for (std::string::size_type i(0); i != s1.size(); ++i)
{
s2 += islower(s1[i]) ? toupper(s1[i]) : s1[i];
}
To shorten this still further, you don't really need the test since
tolower() will return non-alphabetic characters unmodified so we would
simply have s2 += toupper(s1[i]) which has the benefit of not
generating so many temporaries.
You could also avoid calling s1.size() each time round the loop (since
it is invariant) by declaring a constant like
std::string::size_type const s1sz(s1.size());
and then writing the loop as
for (std::string::size_type i(0); i != s1sz; ++i)
....
How to avoid the type of mistake you made? Practice. Code walkthroughs
to see where improvements could be made. Having your code checked by a
partner. Submitting your code to a forum for critique if, like me, you
are a hobbyist and on your own. Check your code against the standard
(the draft is online if you don't have an official copy), though
reading "standardese" is not easy. Get a good tutorial book written in
the modern style (I suggest Accelerated C++ by Andrew Koenig, You Can
Program in C++ by Francis Glassborow and The C++ Programming Language
by Bjarne Stroustrup although the latter isn't wonderful as a tutorial
for beginners but is a good reference)
Hope this helps.
Cheers
Jim..
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]