Simplify this ?

[Flzw]

Alright, here is a simple function I coded, won't be hard understanding what
it does, and YES, I know, you will probably tell me it's awful code, I would
like to know how to write it better, maybe using stringstream ? here it is :

std::string IntToDiskSpace( int size)
{
if (size < 1024)
return IntToStr( size ) + " Bytes";
else if (size < 1048576)
return IntToStr( size / 1024) + "KB";
else if (size < 1073741824)
{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1048576);
string result = temp;
result += " MB";
delete temp;
return result;
}
else
{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1073741824);
string result = temp;
result += " GB";
delete temp;
return result;
}
}

[Mike Wahler]

"Flzw" <fl****@wanadoo .fr> wrote in message
news:41******** **************@ news.wanadoo.fr ...

Alright, here is a simple function I coded, won't be hard understanding what it does,
No, we cannot know what it does, since it calls a function
whose definition you don't show.
and YES, I know, you will probably tell me it's awful code,
Well, it's incomplete code. It won't compile. If you want
folks to assess it, it's much better to post something compilable
if at all possible.
I would
like to know how to write it better, maybe using stringstream ? here it is

:

I suggest you tell us, in English, what you want it to do.
And yes, I'd get rid of the e.g. 'sprintf()'s as there
are safer, more robust ways to generate strings.

I'd also dump the unnecessary dynamic allocations.

-Mike

[Flzw]

Allright, as Mike pointed out, there was not the code for another function I
made but that was irrelevant in the questions I had but yes I agree that was
not necessarily obvious, so let's reduce the code to this :

std::string IntToDiskSpace( int size)
{
if (size < 1073741824)
{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1048576);
string result = temp;
result += " MB";
delete temp;
return result;
}
else
{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1073741824);
string result = temp;
result += " GB";
delete temp;
return result;
}
}

I know I shouldn't use sprintf (thus the "unecessary dynamic allocations"
but I'm unsure on how to dit, I guess stringstream or stringbuf could help
there but these are quite obscure for me...

[Victor Bazarov]

"Flzw" <fl****@wanadoo .fr> wrote...

Alright, here is a simple function I coded, won't be hard understanding
what
it does, and YES, I know, you will probably tell me it's awful code,
It's not awful. It's just full of assumptions which are not necessarily
correct, and some undefined behaviour. Just a little bit of undefined
behaviour, not a whole lot. Well, actually, more than a little bit.
Yeah, it's awful.
I would
like to know how to write it better, maybe using stringstream ? here it is
:

std::string IntToDiskSpace( int size)
{
if (size < 1024)
return IntToStr( size ) + " Bytes";
else if (size < 1048576)
return IntToStr( size / 1024) + "KB";
else if (size < 1073741824)
1073741824 is a literal that doesn't necessarily fit into an int. You might
want to consider using 'L' suffix.
{
char* temp = new char[64];
You really don't need to dynamically allocate 'temp'. Just declare the
array as automatic:

char temp[64];
sprintf( temp, "%2f", size / 1048576);
You probably wanted to print decimal points, the format you need is '%.2f'.
Second, the 'size / 1048576' has the type 'int', so if you have %f for
the format, the 'sprintf' expects a double value. So, you _need_ to supply
a double value there, otherwise its behaviour is undefined:

sprintf(temp, "%.2f", size / 1048576.);
string result = temp;
result += " MB";
delete temp;
Another bit of undefined behaviour. If you allocate using 'new[]', you
MUST free it using 'delete[]':

delete[] temp;

Of course, if you follow my advice to use a simple automatic array, you
don't need to delete at all.
return result;
}
else
{
Same notes in this block as above.
char* temp = new char[64];
sprintf( temp, "%2f", size / 1073741824);
string result = temp;
result += " GB";
delete temp;
return result;
}
}

A simplified version might begin like this

std::ostringstr eam os;
if (size < 1024)
return (os << size << " Bytes").str();
size /= 1024;
if (size < 1024)
return (os << size << " KB").str();
...

Victor

[Alf P. Steinbach]

* Flzw:

Alright, here is a simple function I coded, won't be hard understanding what
it does, and YES, I know, you will probably tell me it's awful code, I would
like to know how to write it better, maybe using stringstream ? here it is :
First thing is to make it _correct_ as per current standards.

1024 bytes is not 1 KB. It's 1 KiB.

2^20 bytes is not 1 MB. It's 1 MiB.

std::string IntToDiskSpace( int size)
'int' is unlikely to be able to represent modern disk capacitites.

{
if (size < 1024)
return IntToStr( size ) + " Bytes";
else if (size < 1048576)
return IntToStr( size / 1024) + "KB";
else if (size < 1073741824)
Instead of decimal numbers, use values derived from expressions
such as

size_t const K = (1 << 10); // 1024
size_t const M = K*K;
size_t const G = K*M;
{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1048576);

Oops.

Check out boost::lexical_ cast. See <url: http://www.boost.org>.

When you get things _working_ and _correct_ you can start to
worry about optimization -- until then forget optimization!

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[Victor Bazarov]

"Flzw" <fl****@wanadoo .fr> wrote...

Allright, as Mike pointed out, there was not the code for another function
I made but that was irrelevant in the questions I had but yes I agree that
was not necessarily obvious, so let's reduce the code to this :

std::string IntToDiskSpace( int size)
{
if (size < 1073741824)
Another advice (in addition to my post): typos are programmer's worst
enemies.
It's quite possible to make a mistake in such a constant even if you copy
and
paste it from a calculator or something. I recommend using 1024*1024*1024
instead. The compiler will calculate it for you. Besides, it's readable a
bit
better than 1034234582 (oops.. I guess I mistyped)
[...]

[Victor Bazarov]

"Alf P. Steinbach" <al***@start.no > wrote...

* Flzw:

Alright, here is a simple function I coded, won't be hard understanding
what
it does, and YES, I know, you will probably tell me it's awful code, I
would
like to know how to write it better, maybe using stringstream ? here it
is :
First thing is to make it _correct_ as per current standards.

1024 bytes is not 1 KB. It's 1 KiB.

2^20 bytes is not 1 MB. It's 1 MiB.

And which OS do you know that reports file sizes in "KiB" or "MiB"?
Besides, isn't "MiB" a trademark or something for "Men In Black, The"?

std::string IntToDiskSpace( int size)

'int' is unlikely to be able to represent modern disk capacitites.

{
if (size < 1024)
return IntToStr( size ) + " Bytes";
else if (size < 1048576)
return IntToStr( size / 1024) + "KB";
else if (size < 1073741824)

Instead of decimal numbers, use values derived from expressions
such as

size_t const K = (1 << 10); // 1024
size_t const M = K*K;
size_t const G = K*M;

{
char* temp = new char[64];
sprintf( temp, "%2f", size / 1048576);

Oops.

Check out boost::lexical_ cast. See <url: http://www.boost.org>.

Why? Couldn't _standard_ means be used?
When you get things _working_ and _correct_ you can start to
worry about optimization -- until then forget optimization!

He didn't ask for optimization. He asked to simplify. That's mostly
related to _readability_ and _maintainabilit y_ and not optimization.

V

[Alf P. Steinbach]

* Victor Bazarov:

"Alf P. Steinbach" <al***@start.no > wrote...

* Flzw:

Alright, here is a simple function I coded, won't be hard understanding
what
it does, and YES, I know, you will probably tell me it's awful code, I
would
like to know how to write it better, maybe using stringstream ? here it
is :
First thing is to make it _correct_ as per current standards.

1024 bytes is not 1 KB. It's 1 KiB.

2^20 bytes is not 1 MB. It's 1 MiB.

And which OS do you know that reports file sizes in "KiB" or "MiB"?

Some Linux utilities and desktops do, not that I know them intimately
or can provide references out of hand.

The situation for Windows is, however, as always: everything's
non-standard, including the terminology, units, etc.

For simple information about the international standard, see
<url: http://physics.nist.go v/cuu/Units/binary.html>.

Besides, isn't "MiB" a trademark or something for "Men In Black, The"?
He he he... :-)

You got me laughing.

Somebody should notify Hollywood -- great lawsuit!

Check out boost::lexical_ cast. See <url: http://www.boost.org>.

Why? Couldn't _standard_ means be used?

boost::lexical_ cast is a simple wrapper around the standard means,
namely a std::stringstre am-based conversion -- check it out... ;-)

When you get things _working_ and _correct_ you can start to
worry about optimization -- until then forget optimization!

He didn't ask for optimization. He asked to simplify. That's mostly
related to _readability_ and _maintainabilit y_

Yes. We agree! :-)
and not optimization.

Nope. All the code's ugliness stems from misguided attempts at
optimization, e.g. using sprintf instead of iostreams. Where the
possible benefit is more than cancelled by the overhead of 'new'.

--
A: Because it messes up the order in which people normally read text.
Q: Why is it such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

[Flzw]

It's not awful. It's just full of assumptions which are not necessarily
correct, and some undefined behaviour. Just a little bit of undefined
behaviour, not a whole lot. Well, actually, more than a little bit.
Yeah, it's awful.

Thought so ;)

1073741824 is a literal that doesn't necessarily fit into an int. You
might
want to consider using 'L' suffix.
Well, it's only formy own use on a personal project compiled with a compiler
with 32bits ints, so it does fit in, I did this on purpose, but I changed
anyway, dividing by 1024 each time will probably be faster.
You really don't need to dynamically allocate 'temp'. Just declare the
array as automatic:

char temp[64];

good point, like the 2 others, we'll blame the delete[] stuff on fatigue

A simplified version might begin like this

std::ostringstr eam os;
if (size < 1024)
return (os << size << " Bytes").str();
size /= 1024;
if (size < 1024)
return (os << size << " KB").str();
...

Allrigth this is it, I tried something like this :
string IntToDiskSpace( int size)
{
std::ostringstr eam os;
double fsize = size;

if (fsize < 1024.0)
return (os << size << " Bytes").str();
fsize /= 1024.0;
if (fsize < 1024.0)
return (os << size / 1024 << " KB").str();
fsize /= 1024.0;
if (fsize < 1024.0)
return (os << ios::fixed() << setprecision(2) << fsize << " MB").str();
fsize /= 1024.0;
return (os << fixed << setprecision(2) << fsize << " GB").str();
}

There are probably mistakes in this too, but the compiler gives me one on
the first line, and I don't understand it :

error C2079: 'os' uses undefined class
'std::basic_ost ringstream<_Ele m,_Traits,_Allo c>'
with
[
_Elem=char,
_Traits=std::ch ar_traits<char> ,
_Alloc=std::all ocator<char>
]

etc ...
I can't find why....

[Flzw]

error C2079: 'os' uses undefined class
'std::basic_ost ringstream<_Ele m,_Traits,_Allo c>'
with
[
_Elem=char,
_Traits=std::ch ar_traits<char> ,
_Alloc=std::all ocator<char>
]

etc ...
I can't find why....

#include <sstream> right =)