Please critique this function

[Daniel T.]

The function below does exactly what I want it to (there is a main to
test it as well.) However, I'm curious about ideas of making it better.
Anyone interested in critiquing it?

void formatText( const string& in, int charsPerLine, int lines,
vector< string >& out )
{
out.resize( 1 );
out[0] = "";
int prev = 0;
int pos = charsPerLine;
int lineCount = 0;
while ( pos < in.size() ) {
pos = in.find_last_of ( " -", pos );
if ( pos < prev ) {
pos = prev + charsPerLine - 1;
out.back() += in.substr( prev, pos - prev );
out.back() += '-';
prev = pos;
}
else if ( in[pos] == '-' ) {
++pos;
out.back() += in.substr( prev, pos - prev );
prev = pos;
}
else {
out.back() += in.substr( prev, pos - prev );
prev = pos + 1;
}
out.back() += (char)0x0A;
pos += charsPerLine;
++lineCount;
if ( lines <= lineCount ) {
out.back().eras e( out.back().size () - 1 );
out.push_back( "" );
lineCount = 0;
}
}
out.back() += in.substr( prev );
}

int main()
{
string in = "hello";
vector< string out;
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "hello" );

in = "Put up with it and you will get more of it.";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Put up with it and you\nwill get more of it." );

in = "The mind of a bigot is like the pupil of the eye. The more
light you shine on it, the more it will contract.";
formatText( in, 24, 4, out );
assert( out.size() == 2 );
assert( out[0] == "The mind of a bigot is\nlike the pupil of
the\neye. The more light you\nshine on it, the more" );
assert( out[1] == "it will contract." );

in = "Floccinaucinih ilipili-fication";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Floccinaucinih ilipili-\nfication" );

in = "Floccinaucinih ilipilification ";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Floccinaucinih ilipilifi-\ncation" );

}

[Daniel T.]

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

However, I'm curious about ideas of making it better.
Anyone interested in critiquing it?

void formatText( const string& in, int charsPerLine, int lines,
vector< string >& out )
{
out.resize( 1 );
out[0] = "";
int prev = 0;
int pos = charsPerLine;
int lineCount = 0;
while ( pos < in.size() ) {
pos = in.find_last_of ( " -", pos );
if ( pos < prev ) {
pos = prev + charsPerLine - 1;
out.back() += in.substr( prev, pos - prev );
out.back() += '-';
prev = pos;
}
else if ( in[pos] == '-' ) {
++pos;
out.back() += in.substr( prev, pos - prev );
prev = pos;
}
else {
out.back() += in.substr( prev, pos - prev );
prev = pos + 1;
}
out.back() += (char)0x0A;
pos += charsPerLine;
++lineCount;
if ( lines <= lineCount ) {
out.back().eras e( out.back().size () - 1 );
out.push_back( "" );
lineCount = 0;
}
}
out.back() += in.substr( prev );
}

I rewrote it as follows, but I'm afraid that removed the desired UB:

I modified your re-write so it passes all the tests:

void formatText(
string const& in,
int charsPerLine,
int lines,
vector< string >& out )
{
vector<string result( 1 );

size_t prev = 0;
size_t pos = charsPerLine;
int lineCount = 0;
while ( pos < in.size() )
{
pos = in.find_last_of ( " -", pos );
if ( pos == string::npos || pos < prev )
{
pos = prev + charsPerLine - 1;
result.back() += in.substr( prev, pos - prev );
result.back() += '-';
prev = pos;
}
else if ( in.at( pos ) == '-' )
{
++pos;
result.back() += in.substr( prev, pos - prev );
prev = pos;
}
else
{
result.back() += in.substr( prev, pos - prev );
prev = pos + 1;
}
result.back() += '\n';
pos += charsPerLine;
++lineCount;
if ( lines <= lineCount )
{
result.back().e rase( result.back().s ize() - 1 );
result.push_bac k( "" );
lineCount = 0;
}
}
result.back() += in.substr( prev );
out.swap( result );
}

I see two basic differences in your code from mine...

1) You define 'pos' and 'prev' as unsigned values which exposed the UB.

2) You do all the work in the vector 'result' rather than the 'out'
parameter. I assume that is to insure that 'out' is unchanged if the
function throws?

The logic, however seems identical to mine. Can you think of any
improvements to the logic?

[James Kanze]

On Oct 22, 12:11 am, "Daniel T." <danie...@earth link.netwrote:

The function below does exactly what I want it to (there is a main to
test it as well.) However, I'm curious about ideas of making it better.
Anyone interested in critiquing it?

Well, just from the code, I have great difficulty even
understanding it. I'm not even sure what it is supposed to do.

If you're trying to break up text into lines, I don't really
understand the output; what is each entry in the vector (since
the entries themselves have line breaks)? Without a
specification of what the function is supposed to do, you can't
begin to write it, write a test for it, or ask anyone to review
it.

Otherwise: in general, if the problem is breaking up text into
lines of a maximum width, with line breaks only at word
boundaries (or other specified places), then the usual solution
would be to break the problem up into parts: a class which
breaks the input up into words, and a class which puts the words
into the destintation, adding line breaks as needed.

--
James Kanze (GABI Software) email:ja******* **@gmail.com
Conseils en informatique orientée objet/
Beratung in objektorientier ter Datenverarbeitu ng
9 place Sémard, 78210 St.-Cyr-l'École, France, [phone]

[Daniel T.]

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

* Daniel T. -Alf P. Steinbach:

The logic, however seems identical to mine. Can you think of any
improvements to the logic?

formatText( "123456 123456", 6, 4, result );

Thanks. Not only did it show an anomaly in the code, but it exposed an
error in one of the other tests! Still no ideas on reducing the
complexity of the code though?

void formatText(
string const& in,
int charsPerLine,
int lines,
vector< string >& out )
{
vector<string result( 1 );

size_t prev = 0;
size_t pos = charsPerLine;
int lineCount = 0;
while ( pos < in.size() )
{
pos = in.find_last_of ( " -", pos );
if ( pos == string::npos || pos < prev )
{
pos = prev + charsPerLine - 1;
result.back() += in.substr( prev, pos - prev );
result.back() += '-';
prev = pos;
}
else if ( in.at( pos ) == '-' )
{
++pos;
result.back() += in.substr( prev, pos - prev );
prev = pos;
}
else
{
result.back() += in.substr( prev, pos - prev );
prev = pos + 1;
}
result.back() += '\n';
pos = prev + charsPerLine;
++lineCount;
if ( lines <= lineCount )
{
result.back().e rase( result.back().s ize() - 1 );
result.push_bac k( "" );
lineCount = 0;
}
}
result.back() += in.substr( prev );
out.swap( result );
}

int main()
{
string in = "hello";
vector< string out;
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "hello" );

in = "Put up with it and you will get more of it.";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Put up with it and you\nwill get more of it." );

in = "The mind of a bigot is like the pupil of the eye. The more
light you shine on it, the more it will contract.";
formatText( in, 24, 4, out );
assert( out.size() == 2 );
assert( out[0] == "The mind of a bigot is\nlike the pupil of
the\neye. The more light you\nshine on it, the more it" );
assert( out[1] == "will contract." );

in = "Floccinaucinih ilipili-fication";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Floccinaucinih ilipili-\nfication" );

in = "Floccinaucinih ilipilification ";
formatText( in, 24, 4, out );
assert( out.size() == 1 );
assert( out[0] == "Floccinaucinih ilipilifi-\ncation" );

formatText( "123456 123456", 6, 4, out );
assert( out.size() == 1 );
assert( out[0] == "123456\n123456 " );

}

[Daniel T.]

James Kanze <ja*********@gm ail.comwrote:

On Oct 22, 12:11 am, "Daniel T." <danie...@earth link.netwrote:

The function below does exactly what I want it to (there is a
main to test it as well.) However, I'm curious about ideas of
making it better. Anyone interested in critiquing it?

Well, just from the code, I have great difficulty even
understanding it. I'm not even sure what it is supposed to do.

That's why I posted it. I don't generally make functions with that high
a complexity and so I was wondering if someone could make some
suggestions on the logic to see if the complexity can be reduced.

As far as what it's supposed to do... It is supposed to pass all the
tests that were included. I.E., proper execution will exit normally, if
an assert fires, then there is a problem. (Note, due to Alf's helpful
critique I have modified one of the tests and added another.)

If you're trying to break up text into lines, I don't really
understand the output; what is each entry in the vector (since the
entries themselves have line breaks)?

Each entry in the vector is a page.

Without a specification of what the function is supposed to do, you
can't begin to write it, write a test for it, or ask anyone to
review it.

The function is supposed to take a string of text and break it up into a
number of pages. Each page is supposed to be no more than charsPerLine
wide and no more than lines high. The code must be able to hyphenate any
word wider than charsPerLine (although not necessarily at the proper
place according to English grammar,) and the function must be able to
recognize when a very long word is already hyphenated so it won't add
unnecessary hyphens.

Otherwise: in general, if the problem is breaking up text into
lines of a maximum width, with line breaks only at word boundaries
(or other specified places), then the usual solution would be to
break the problem up into parts: a class which breaks the input up
into words, and a class which puts the words into the destintation,
adding line breaks as needed.

Can you show me code demonstrating this? Would such code be less complex
than what I already have? I'm interested in reducing the cyclomatic
complexity if possible.

[Daniel T.]

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

* Daniel T.:

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

* Daniel T. -Alf P. Steinbach:

The logic, however seems identical to mine. Can you think of any
improvements to the logic?

formatText( "123456 123456", 6, 4, result );

Thanks. Not only did it show an anomaly in the code, but it exposed an
error in one of the other tests! Still no ideas on reducing the
complexity of the code though?

That /was/ my idea for simplifying the code: should be much simpler if
you fix that.

I fixed that, but my fix didn't change the cyclomatic complexity...

I fail to see any changes in the formatText function (but then, I only
glanced at it, I presume you'd have explained it if you fixed it)?

The only thing I can think to change is that the function performs two
different jobs through the loop. It breaks the string up into lines, and
it breaks lines up into pages. I could seperate these two jobs into two
functions; "breakLines " and "breakPages ". Oh well, I have other
functions to write. Thanks for the help.

[Kai-Uwe Bux]

Daniel T. wrote:

James Kanze <ja*********@gm ail.comwrote:

>On Oct 22, 12:11 am, "Daniel T." <danie...@earth link.netwrote:

The function below does exactly what I want it to (there is a
main to test it as well.) However, I'm curious about ideas of
making it better. Anyone interested in critiquing it?

Well, just from the code, I have great difficulty even
understandin g it. I'm not even sure what it is supposed to do.

That's why I posted it. I don't generally make functions with that high
a complexity and so I was wondering if someone could make some
suggestions on the logic to see if the complexity can be reduced.

As far as what it's supposed to do... It is supposed to pass all the
tests that were included. I.E., proper execution will exit normally, if
an assert fires, then there is a problem. (Note, due to Alf's helpful
critique I have modified one of the tests and added another.)

You cannot really be serious. The following passes all the tests. Moreover,
the logic that makes is pass all the tests is very simple and easy to
understand. Nonetheless, I seriously doubt that it will satisfy you:
struct text : vector< string {
text & add ( string const & line ) {
push_back( line );
return ( *this );
}
};

struct arguments : pair< string, pair< int, int {
arguments( string const & line, int a, int b )
: pair< string, pair< int, int ( line, pair<int,int>(a ,b) )
{}
};

void formatText (
string const& in,
int charsPerLine,
int lines,
vector< string >& out )
{
map< arguments, text table;
table[ arguments( "hello", 24, 4 ) ]
.add( "hello" );
table[ arguments( "Put up with it and you will"
" get more of it.", 24, 4 ) ]
.add("Put up with it and you\nwill get more of it.");
table[ arguments( "The mind of a bigot is like the pupil"
" of the eye. The more light you shine"
" on it, the more it will contract.", 24, 4 ) ]
.add("The mind of a bigot is\nlike the pupil"
" of the\neye. The more light you\nshine"
" on it, the more it")
.add("will contract.");
table[ arguments( "Floccinaucinih ilipili-fication", 24, 4 ) ]
.add("Floccinau cinihilipili-\nfication");
table[ arguments( "Floccinaucinih ilipilification ", 24, 4 ) ]
.add("Floccinau cinihilipilifi-\ncation");
table[ arguments( "123456 123456", 6,4 ) ]
.add("123456\n1 23456");

text result = table[ arguments( in, charsPerLine, lines ) ];
out = result;
}

>If you're trying to break up text into lines, I don't really
understand the output; what is each entry in the vector (since the
entries themselves have line breaks)?

Each entry in the vector is a page.

>Without a specification of what the function is supposed to do, you
can't begin to write it, write a test for it, or ask anyone to
review it.

The function is supposed to take a string of text and break it up into a
number of pages. Each page is supposed to be no more than charsPerLine
wide and no more than lines high. The code must be able to hyphenate any
word wider than charsPerLine (although not necessarily at the proper
place according to English grammar,) and the function must be able to
recognize when a very long word is already hyphenated so it won't add
unnecessary hyphens.

Ah. Thanks. That helps.

I guess a first step to break up the complexity is to split the function in
two: one splits the text into lines, the next collates lines into pages.
Something like:

void split_into_line s ( string const & in,
unsigned int charsPerLine,
vector< string & out );

void collate_pages ( vector< string const & in,
unsigned int linesPerPage,
vector< string & out );
Then you'd have:

void formatText(
string const& in,
unsigned int charsPerLine,
unsigned int linesPerPage,
vector< string >& out ) {
vector< string lines;
split_into_line s( in, charsPerLine, lines );
collate_into_pa ges( lines, linesPerPage, out );
}

>Otherwise: in general, if the problem is breaking up text into
lines of a maximum width, with line breaks only at word boundaries
(or other specified places), then the usual solution would be to
break the problem up into parts: a class which breaks the input up
into words, and a class which puts the words into the destintation,
adding line breaks as needed.

Can you show me code demonstrating this? Would such code be less complex
than what I already have? I'm interested in reducing the cyclomatic
complexity if possible.

What is this cyclomatic complexity (and why is it bad)?
Best

Kai-Uwe Bux

[Daniel T.]

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

I'm not sure, but in this equivalent expression of the function it seems
as if it might have a off-by-one bug.

That has me worried. I've hit the function with several tests but have
been unable to find any problems so far. Why do you feel there is an off
by one bug? Is there something specific, or is it just a gut feeling?

[Daniel T.]

Kai-Uwe Bux <jk********@gmx .netwrote:

What is this cyclomatic complexity (and why is it bad)?

http://www.literateprogramming.com/mccabe.pdf

[=?UTF-8?B?RXJpayBXaWtzdHLDtm0=?=]

On 2007-10-22 13:38, Daniel T. wrote:

James Kanze <ja*********@gm ail.comwrote:

>On Oct 22, 12:11 am, "Daniel T." <danie...@earth link.netwrote:

Can you show me code demonstrating this? Would such code be less complex
than what I already have? I'm interested in reducing the cyclomatic
complexity if possible.

A bit off-topic but in my experience there is no benefit in reducing the
cyclomatic complexity just for the sake of reducing it. A toolthat
automatically calculates the cyclomatic complexity can be useful since
it helps you find functions that are *potentially* too complex. But each
function has to be evaluated by a human to determine if it is too
complex or not, i.e. a function with a large switch-statement will have
a high cyclomatic complexity but will not necessarily be very complex.

After a quick look at your function it looks like it would have a
cyclomatic complexity of about 5, which IMO is not very high.

BTW: you use the following to add a line-break: "out.back() +=
(char)0x0A;" but would it not be better to use "out.back() += '\n';" to
make the code more portable?

--
Erik Wikström