ansaurus

Question

Answer 1

+5 A:

You have four specifiers in your format string, yet are only supplying three additional arguments.

Anon. 2010-01-21 21:29:25

Answer 2

+3 A:

The only thing that looks totally broken is that you're only passing 3 data parameters to your sprintf, but it's expecting 4 (%c, %s, %s, %s)

Joe 2010-01-21 21:30:55

The first format specifier for sprintf is broken.

John Dibling 2010-01-21 22:47:21

Answer 3

+13 A:

sprintf has 4 place holders while you give only 3 parameters.

I would suggest:

string MonteCarloBasketDistribution::fileName(char c) {
   std::ostringstream result;
   result << c <<"_"<<copula<<'('<<left<<", "<<right<<").csv";
   return result.str();
}

Your sprintf is not safe for buffer overflow, use rather C99 snprintf or std::stringstream

Artyom 2010-01-21 21:33:25

Generally agree, except that since everything he presented is character based, streams are kinda overkill.

T.E.D. 2010-01-21 21:38:14

The point is that `toString()` in C++ is `<<`. It is just wrong to provide `toString()` members -- it is not Java or C#. Iostreams much more flexible.

Artyom 2010-01-21 22:13:27

The decision to use `sprintf` or streams is in large part a matter of choice. Personally, I hate streams. I find `sprintf` to be much more consise and easier to have total control of the output.

John Dibling 2010-01-21 22:49:34

@John Except now you can't output in generic code. Oops.

GMan 2010-01-22 00:26:31

@Gman. That's OK. There is not one almighty paradigm. :)

John Dibling 2010-01-22 02:05:55

@John: Perhaps not universally, but evidence points to generic code being one of the best methods of achieving a task. Especially in C++. :)

GMan 2010-01-22 04:16:02

It is fine to use `*printf` familiy, but use`snprintf` instead of `sprintf`... Whitch is much safer, but Oooops... MSVC does not support C99 snprintf... So if you on MSVC use `iostreams`

Artyom 2010-01-22 07:33:31

BTW, `boost::format` uses `iostreams` internally.

Artyom 2010-01-22 07:34:45

@Artyom - I have to disagree a bit. `<<` is more like 'toStream()'. Sure, they are convertable. But if you actually want a string for some reason, rather than a stream (eg: Building a file name for opening), then I'd much rather just use a toString() call, and not have to declare some bogus temporary stream object that isn't in and of itself useful for anything.

T.E.D. 2010-01-22 19:31:58

The point, that there is no toString() for every object in C++ like it is in Java/C#, because there is no single inheritence tree. So rather then mimicing Java/C# interface, you should use native C++ ideoms. You want `toString` use `boost::lexical_cast<std::string>` it does exactly what `toString` should.

Artyom 2010-01-22 20:38:18

Ah. See, I don't use those languages and aren't that familar with them (yet anyway...), so that point was kinda lost on me. That being said, I'm not real fond of C++'s use of streams as some kind of universal string conversion pardigm. They are too inconvienent for that. It would have been much better to use them as a replacement for general file I/O, and use std::string and overloads of the "+" operator (instead of the "<<" operator) as C++'s "toString()". Shame they didn't quite go that way.

T.E.D. 2010-01-27 16:29:57

Answer 4

+12 A:

Since all the things you are tacking together are character-based, using sprintf for it is kinda silly.

What a C++ programmer should be doing there is something more along the lines of:

std::string result = copula.toString() + "(" + left.toString() + "," 
                   + right.toString() + ")";

T.E.D. 2010-01-21 21:34:53

With the caveat that there are situations in which using an ostringstream is a much better idea. In this case, this code is likely the right thing.

Omnifarious 2010-01-21 21:44:46

"Should" is awfully subjective and a matter of style.

John Dibling 2010-01-21 22:50:24

It's sometimes useful to have a separate and clear format specifier, even when you're only dealing with strings. I find that long cluttered sequences of string concatenations are painful on the eyes. With something like "%c_%s(%s, %s).csv" you can immediately visualize what the formatting rule is. By using Boost.Format, you get the benefit of format specifiers without the dangers of missing/invalid parameters.

Emile Cormier 2010-01-21 23:06:49

@Omnifarious: Agreed.

T.E.D. 2010-01-21 23:23:16

@Emile: Once I got trained at it, I'll admit that the C format specs are the easiest to read option. However, they are so incredibly error prone (at least for me), that I've sworn off of them except under extreme circumstances. It was not uncommon for me to waste *hours* tracking down supposed bad data that just ended up being a format error very much like the one Grzenio made above.

T.E.D. 2010-01-21 23:28:43

@TED: If it weren't for Boost.Format, I'd agree with you 100%. I wouldn't touch snprintf with a 10-foot pole unless I absolutely had to. :-)

Emile Cormier 2010-01-22 01:05:36

Answer 5

+4 A:

Since you seem to work on std::string, you needn't use sprintf at all. std::string has simple to use overloaded operators, so you can concantenate strings using +=. I think + works too, so just "add" std::strings together.

mingos 2010-01-21 21:37:12

Answer 6

+2 A:

Assuming that toString() returns a std::string,

This:

sprintf(result, "%c_%s(%s, %s).csv", copula.toString().c_str(), left.toString().c_str(), right.toString().c_str());

...should be:

sprintf(result, "%s_%s(%s, %s).csv", copula.toString().c_str(), left.toString().c_str(), right.toString().c_str());

John Dibling 2010-01-21 22:46:54

John, you are probably correct that the %s should be first instead of the %c, but maybe he wanted just the first character of the string. If so he was still wrong--he was passing the address of the string rather than the first char.What you really missed here mis-match on the number of arguments passed into the sprintf, three, when the specifier requires four.Minus one for you, if I could vote.

aaaa bbbb 2010-01-21 23:20:25

Answer 7

+3 A:

Boost has a formatting library that is safer than printf and friends.

#include <boost/format.hpp>
string MonteCarloBasketDistribution::fileName(char c)
{
    return boost::str( boost::format("%c_%s(%s, %s).csv")
        % c % copula.toString() % left.toString() % right.toString() );
}

or, alternatively:

#include <boost/format.hpp>
string MonteCarloBasketDistribution::fileName(char c)
{
    return boost::str( boost::format("%1%_%2%(%3%, %4%).csv")
        % c % copula.toString() % left.toString() % right.toString() );
}

With the latter example, Boost knows that %1% is a character and that %2% thru %4% are strings by "looking" at the types of the arguments passed in via the % operator.

Emile Cormier 2010-01-21 22:48:26

:O Boost is surprising me all the time.

qba 2010-01-21 23:00:39

Whenever there seems to be a generally useful feature missing from the language or the standard library, chances are it has been implemented in Boost. :-)

Emile Cormier 2010-01-21 23:04:10

How does boost deal with the first argument being a %c and the thing passed to it is the address of a string, not a char?

aaaa bbbb 2010-01-21 23:25:37

@aaaa bbbb: It throws a format_error exception.

Emile Cormier 2010-01-22 01:08:40

ansaurus

tags:

views:

answers:

Concatenating strings in C++

related questions