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]

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

 

[Victor Bazarov]

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:stringstream os;
if (size < 1024)
return (os << size << " Bytes").str();
size /= 1024;
if (size < 1024)
return (os << size << " KB").str();
...

Victor

 

[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...

 

[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!

 

[Victor Bazarov]

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]

* Flzw:

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 _maintainability_ and not optimization.

V

 

[Alf P. Steinbach]

* Victor Bazarov:

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

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!

Why? Couldn't _standard_ means be used?

boost::lexical_cast is a simple wrapper around the standard means,
namely a std::stringstream-based conversion -- check it out... ;-)

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

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'.

 

[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:stringstream 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:stringstream 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_ostringstream<_Elem,_Traits,_Alloc>'
with
[
_Elem=char,
_Traits=std::char_traits<char>,
_Alloc=std::allocator<char>
]

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

 

[Flzw]

error C2079: 'os' uses undefined class
'std::basic_ostringstream<_Elem,_Traits,_Alloc>'
with
[
_Elem=char,
_Traits=std::char_traits<char>,
_Alloc=std::allocator<char>
]

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

#include <sstream> right =)

 

[Karl Heinz Buchegger]

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

Whenever you see 'uses undefined class' the compiler has
encountered a name which was not introduced to it earlier.

Basically there are 2 reasones for that.
You either
* have not included everything you should
or
* you made a simple typo.

Since the compiler complains about something not written
by yourself, it most likely is not a typo but a missing
include.

 

[Howard]

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

That's just plain weird! We're using "MiB" for "megabyte" now? Exactly
where did the "i" come from???

-Howard

 

[White Wolf]

That's just plain weird! We're using "MiB" for "megabyte" now? Exactly
where did the "i" come from???

Man in Black. Yoy should go to movies more often. ;-) MB is 2^20bytes, it
is only the cheapo hard-disk manufacturers who are misusing it.

 

[Victor Bazarov]

That's just plain weird! We're using "MiB" for "megabyte" now? Exactly
where did the "i" come from???

It's the 8th letter of the Latin alphabet, probably adopted by
Romans from the Greek "iota". So, a Mega-i-Byte is an iota bigger
than a MegaByte.

 

[Victor Bazarov]

It's the 8th letter of the Latin alphabet, probably adopted by
Romans from the Greek "iota". So, a Mega-i-Byte is an iota bigger
than a MegaByte.

Scratch that. Should read: a Mebibyte is an iota bigger than a Megabyte.

 

[Mike Wahler]

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"?

I suspect so (if I recall correctly, those guys were
in the business of controlling (alien) bugs.)

-Mike

 

[Mike Wahler]

Scratch that. Should read: a Mebibyte is an iota bigger than a Megabyte.

Write a program to calculate and
output the value of an iota.

Don't use atoi().



-Mike

 

[Mike Wahler]

Megabyte.

Write a program to calculate and
output the value of an iota.

Hmm, that probably works better if I'd
written "an iota".

Don't use atoi().

-Mike

 

[=?ISO-8859-15?Q?Juli=E1n?= Albo]

That's just plain weird! We're using "MiB" for "megabyte" now? Exactly
where did the "i" come from???

The mega prefix binary style is now called mebi, bi for binary.

See http://physics.nist.gov/cuu/Units/binary.html for example.

 

[Old Wolf]

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

That doesn't work. The operator<< functions are
in the basic_ostream class and return a value of
type basic_ostream<....> & , which has no member
function 'str'.