 |
C++Talk.NET
C++ language newsgroups
|
| View previous topic :: View next topic |
| Author |
Message |
Alf P. Steinbach
Guest
|
 Posted: Tue Aug 19, 2003 6:15 pm Post subject: Re: Drawback of the following code |
 |
|
On 19 Aug 2003 08:33:44 -0400, [email]dawood (AT) kuala-lumpur (DOT) oilfield.slb.com[/email] (tiger)
wrote:
| Quote: | I am new to C++. Just had some initial basics in it. After which I
wrote a code below, which will display the bit layout of an integer.
I would like the group to analyze it and then point out the drawbacks
and demerits of the code. As a result of which, I will work upon it
and enhance it once I get all of your
opinion. I guess this is a best way of learning C++. So please help
me.
#include <cstdlib
#include
using namespace std;
#define BYTE 8
|
There are some very rare circumstances where a '#define' of a constant is
meaningful in C++. In this case, it isn't. Use 'const'.
Also, in C++ a "byte" is defined as 'char'.
Thus, the number of bits in a byte is the number of bits in a 'char', which
you can easily compute at compile-time.
Third, it would be better with a meaningful name.
Note that all uppercase names by convention are reserved for macros
(although they're used for constants in Java, that's not the case in C++).
| Quote: |
int main()
{
string s = string();
|
You haven't included the header '
Use 'getline' instead.
| Quote: | double d = strtod(s.c_str(),0);
|
Use 'stringstream' instead.
Check for conversion error.
| Quote: | if ((d>INT_MAX) || (d
cout<<"input number either too large or too smalln";
else {
signed ls = atol(s.c_str());
|
For readibility use 'int' instead of 'signed' (means the same).
| Quote: | int i = 1;
s = string();
unsigned y = 1;
while (i++ <= BYTE*sizeof(ls)) {
|
Avoid side-effects.
Also, avoid post-increment and post-decrement.
Also, choose the right kind of loop, in this case 'for'.
Also, to the extent possible try to declare the 'for' loop control
variable in the loop header, like so:
for( int i = ... ; ...; ++i )
{
...
}
| Quote: | if (ls&y)
s = '1' + s;
|
Use '+=' for efficiency and clarity, reverse the string at the end.
| Quote: | else
s = '0' + s;
y<<=1;
}
cout<<"the bit layout in x :"<
}
system("PAUSE");
|
This is system-specific and prevents automated testing of the program.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
 |
Francis Glassborow
Guest
|
| Posted: Tue Aug 19, 2003 6:15 pm Post subject: Re: Drawback of the following code |
|
|
In article <7cdca484.0308190126.651fe8b8 (AT) posting (DOT) google.com>, tiger
<dawood (AT) kuala-lumpur (DOT) oilfield.slb.com> writes
| Quote: | I am new to C++. Just had some initial basics in it. After which I
wrote a code below, which will display the bit layout of an integer.
I would like the group to analyze it and then point out the drawbacks
and demerits of the code. As a result of which, I will work upon it
and enhance it once I get all of your
opinion. I guess this is a best way of learning C++. So please help
me.
|
However I am not sure this is the right newsgroup for that. Have you
considered alt.comp.lang.learn.c-c++ ?
Now bit patterns are IMO best handled with tools designed to manage
bits.
long value;
// code to initialise value
int const bits_in_long(sizeof(long) * CHAR_BIT);
std::bitset bits<bits_in_long>(value);
for(int i(0); i != bits_in_long; ++i) cout << bits[i];
Now there are a couple of things you might want to check in that code.
--
Francis Glassborow ACCU
64 Southfield Rd
Oxford OX4 1PA +44(0)1865 246490
All opinions are mine and do not represent those of any organisation
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Ron Natalie
Guest
|
| Posted: Tue Aug 19, 2003 11:37 pm Post subject: Re: Drawback of the following code |
|
|
"tiger" <dawood (AT) kuala-lumpur (DOT) oilfield.slb.com> wrote
#defines have no scope. Especially bad is #defining some common word like
BYTE that may inadvertantly be used in another context (BYTE is frequently
either a typedef or other synonym for char).
Further, if you really want to know how many bits in a char, there is a macro
called CHAR_BIT in <cstddef> that has this value.
| Quote: | string s = string();
|
At the best this is redundant.
string s;
will do the same thing (default initialize a string).
You don't check to see if this fails.
| Quote: | double d = strtod(s.c_str(),0);
|
Again you don't check to see if this fails.
| Quote: | if ((d>INT_MAX) || (d<INT_MIN))
|
You could also use numeric_limits
| Quote: | signed ls = atol(s.c_str());
|
atol returns long. "signed ls" is type int. Why not atoi which returns
the right thing? Further, signed is only ever really necessary (and
commonly used) with respect to char. For other types, the absence
of any specifier implies signed. I would have said "int ls" here.
Atol is a really lousy function. It exhibits undefined behavior when
given certain input. Your attempt to check the range by using strtod
first isn't good enough. Imagine if the user typed;
999999999999e-9
This would be within the range of INT_MIN and INT_MAX when treated
as a double, but the parsing will stop at the E when converting as an integer
and the string of 9's preceding it might very well overflow a long.
strtol would be a better function to use (it has defined behavior for any null
terminated string it is given). Further, I'm not sure that there's anything your
program has done so far that:
int ls;
cin >> ls;
wouldn't have done.
| Quote: | int i = 1;
s = string();
unsigned y = 1;
while (i++ <= BYTE*sizeof(ls)) {
|
You have something against for?
for(int i = 0; i < BYTE*sizeof(ls); ++i)
By the way, you have a fencepost error. if you allow byte to count
up to BYTE*sizeof(ls), you will have shifted the bit off the end of y.
(That is if BYTE*sizeof(ls) is 32, you will have done 32 left shifts of 1
and that will yield 0). This will cause an extraneous zero to be added
to your output string.
| Quote: | if (ls&y)
s = '1' + s;
else
s = '0' + s;
|
s = (ls&y ? '1' : '0') + s
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Dhruv
Guest
|
| Posted: Wed Aug 20, 2003 12:13 am Post subject: Re: Drawback of the following code |
|
|
On Tue, 19 Aug 2003 08:33:44 -0400, tiger wrote:
| Quote: | if ((d>INT_MAX) || (d<INT_MIN))
cout<<"input number either too large or too smalln";
else {
|
I *think* that an integer cannot be < INT_MIN or > INT_MAX, but just
conform.
Regards,
-Dhruv.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Ben Hutchings
Guest
|
| Posted: Wed Aug 20, 2003 12:17 am Post subject: Re: Drawback of the following code |
|
|
In article <7cdca484.0308190126.651fe8b8 (AT) posting (DOT) google.com>, tiger wrote:
| Quote: | I am new to C++. Just had some initial basics in it. After which I
wrote a code below, which will display the bit layout of an integer.
I would like the group to analyze it and then point out the drawbacks
and demerits of the code. As a result of which, I will work upon it
and enhance it once I get all of your
opinion. I guess this is a best way of learning C++. So please help
me.
#include <cstdlib
#include
using namespace std;
#define BYTE 8
|
If this is supposed to be the number of bits in a byte, you should
instead include
Alternately you can include <limits> and use
std::numeric_limits<unsigned char>::digits.
| Quote: | int main()
{
string s = string();
|
"= string()" is redundant.
This is probably a mistake. When reading user input, you should
normally read complete lines with std::getline() and then parse the
result, as the behaviour of extractors (operator >>) can be
confusing to a user.
Also you must include <istream> before using any extractor and
<ostream> before using any formatter (operator <<).
| Quote: | double d = strtod(s.c_str(),0);
|
The C++ way is to use std::istringstream instead.
| Quote: | if ((d>INT_MAX) || (d<INT_MIN))
|
Those macros should only be defined in
that. Alternately you can include <limits> and use
std::numeric_limits<int>::min and ...::max.
| Quote: | cout<<"input number either too large or too smalln";
else {
signed ls = atol(s.c_str());
|
If you want an int, why did you convert to double beforehand? Then
you could check the state of the stream to see whether that worked.
| Quote: | int i = 1;
s = string();
|
"s.clear();" would be clearer (no pun intended).
| Quote: | unsigned y = 1;
while (i++ <= BYTE*sizeof(ls)) {
|
I think it would be clearer to use the idiomatic:
for (int i = 0; i != CHAR_BIT * sizeof(ls); ++i) {
| Quote: | if (ls&y)
s = '1' + s;
else
s = '0' + s;
y<<=1;
}
|
It doesn't really matter here, but it is horribly inefficient to
add characters at the front of the string like this. It would be
more efficient to start with the most significant bit and append
characters:
for (unsigned y = 1u << (numeric_limits
y != 0;
y >>= 1)
{
s.append(1, (ls & y) ? '1' : '0');
}
| Quote: | | cout<<"the bit layout in x :"<
|
This doesn't tell you the bit layout in x (which doesn't exist), or
even in ls. What it shows is the two's-complement representation
of ls in logical bit order. The actual bit layout can be discovered
by treating ls as an array of unsigned char:
for (int i = 0; i != sizeof(unsigned); ++i)
{
unsigned char byte =
reinterpret_cast
for (unsigned y =
1u << (numeric_limits
y != 0;
y >>= 1)
{
s.append(1, (byte & y) ? '1' : '0');
}
s.append(1, ' ');
}
| Quote: | }
system("PAUSE");
|
Use "getline(cin, s);" instead, as that's portable.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Dan McLeran
Guest
|
| Posted: Wed Aug 20, 2003 1:48 am Post subject: Re: Drawback of the following code |
|
|
[email]dawood (AT) kuala-lumpur (DOT) oilfield.slb.com[/email] (tiger) wrote in message
news:<7cdca484.0308190126.651fe8b8 (AT) posting (DOT) google.com>...
| Quote: | I am new to C++. Just had some initial basics in it. After which I
wrote a code below, which will display the bit layout of an integer.
|
std::bitset probably will meet your needs. Example:
#include <iostream>
#include <stdlib.h>
#include <bitset>
int main(int argc, char *argv[])
{
using namespace std;
bitset<5> b;
cout<
b |= 5;
cout<
system("PAUSE");
return 0;
}
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Bo Persson
Guest
|
| Posted: Wed Aug 20, 2003 2:51 am Post subject: Re: Drawback of the following code |
|
|
"tiger" <dawood (AT) kuala-lumpur (DOT) oilfield.slb.com> skrev i meddelandet
news:7cdca484.0308190126.651fe8b8 (AT) posting (DOT) google.com...
| Quote: | I am new to C++. Just had some initial basics in it. After which I
wrote a code below, which will display the bit layout of an integer.
I would like the group to analyze it and then point out the
drawbacks
and demerits of the code. As a result of which, I will work upon it
and enhance it once I get all of your
opinion. I guess this is a best way of learning C++. So please
help
me.
#include <cstdlib
#include
|
This is a bit picky, but formally you should also include
and/or <ostream> to get the operators. Often <iostream> includes
these, but it is not required.
| Quote: | using namespace std;
|
Try to avoid this - I assure you that you will soon learn to write
std::cout without any problems.
I have two problems with this. :-)
If you want a constant compatible with sizeof(), you can use
const std::size_t byte_size = 8;
and secondly, we have no guarantee that a byte always *is* 8 bits. You
can find CHAR_BIT in <climits> if you really need the value.
| Quote: |
int main()
{
string s = string();
|
string s;
works just as well.
| Quote: | cin>>s;
double d = strtod(s.c_str(),0);
if ((d>INT_MAX) || (d<INT_MIN))
cout<<"input number either too large or too smalln";
else {
signed ls = atol(s.c_str());
|
'signed' means 'signed int', which is usually written as just 'int'.
atol() returns a 'long' value, so you should probably also store it in
a 'long' or use the atoi() functions.
All of this can of course be avoided by reading the value directly
into the variable:
cin >> ls;
if (cin.fail())
cout << "input operation failed";
else
// ...
Also, I don't understand what ls means. Maybe you could use a better
name?
| Quote: | int i = 1;
s = string();
|
This works well for clearing a string, but even more obvious is
s.clear();
On another level, you are reusing this variable s for another purpose.
In larger programs it often causes confusion when a variable has
different meaning at different places, so it might be better to use an
entirely new variable here.
Here again, it is not at all obvious what y is supposed to be.
| Quote: | while (i++ <= BYTE*sizeof(ls)) {
|
More in the C++ style, this would be
for (size_t i = 0; i != CHAR_BIT * sizeof(ls); ++i)
| Quote: | if (ls&y)
s = '1' + s;
else
s = '0' + s;
y<<=1;
}
cout<<"the bit layout in x :"<
}
system("PAUSE");
return 0;
}
|
Bo Persson
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Alf P. Steinbach
Guest
|
| Posted: Wed Aug 20, 2003 2:53 am Post subject: Re: Drawback of the following code |
|
|
On 19 Aug 2003 14:15:25 -0400, Francis Glassborow <francis (AT) robinton (DOT) demon.co.uk>
wrote:
| Quote: | In article <7cdca484.0308190126.651fe8b8 (AT) posting (DOT) google.com>, tiger
[email]dawood (AT) kuala-lumpur (DOT) oilfield.slb.com[/email]> writes
I would like the group to analyze it and then point out the drawbacks
and demerits of the code. As a result of which, I will work upon it
and enhance it once I get all of your
opinion. I guess this is a best way of learning C++. So please help
me.
However I am not sure this is the right newsgroup for that. Have you
considered alt.comp.lang.learn.c-c++ ?
|
On the contrary, I think this group is precisely the right group, and we
the participants should strive to not give the impression of some kind
of closed forum where only academically interesting questions are legitimate.
That is, I think this group is still precisely the right one for quality
advice, also for beginners, although of course not for fast answers.
On the other hand, the merits of SESE and so forth are, IMO, really not
quite on-topic in this group... ;-)
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Shannon Barber
Guest
|
| Posted: Wed Aug 20, 2003 11:51 pm Post subject: Re: Drawback of the following code |
|
|
[email]dawood (AT) kuala-lumpur (DOT) oilfield.slb.com[/email] (tiger) wrote in message
| Quote: | string s = string();
This is odd, you can just say: |
std::string s;
and it will do the same thing (best case, worst case your code makes a
second string and copies it into s). You can create objects on the
stack in C++.
| Quote: | s = string();
Again, while this gets the job done, it's odd: |
s.clear();
is what's expected
(you could also call itoa() and tell it to give you the result in base
2  )
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Andrei Alexandrescu
Guest
|
| Posted: Fri Aug 22, 2003 1:06 pm Post subject: Re: Drawback of the following code |
|
|
"Alf P. Steinbach" <alfps (AT) start (DOT) no> wrote
| Quote: | On the other hand, the merits of SESE and so forth are, IMO, really not
quite on-topic in this group...
|
You are right of course. Only the /problems/ of SESE are on-topic here  ).
Andrei
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
|
FAQ
Search
Memberlist
Usergroups
Register
Profile
Log in to check your private messages
Log in
