Re: Please critique this function
On Oct 22, 1:38 pm, "Daniel T." <danie...@earthlink.net> wrote:
James Kanze <james.ka...@gmail.com> wrote:
On Oct 22, 12:11 am, "Daniel T." <danie...@earthlink.net> wrote:
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.
But until we know exactly what the function is supposed to do,
how can we make suggestions as to how to implement it.
As I said, my vague impression was that it was to format text by
breaking it up into lines, independently of the line breaks in
the original. But in that case, I don't understand your output.
Either you output a string, with '\n' separating the lines, or
you output a std::vector< std::string >, with a separate line in
each element. I don't understand using an std::vector<
std::string >, and still having '\n' in the elements.
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.)
The simplest implementation which would pass all of your tests
would simply be to look up the input in a pre-calculated array,
with the pre-defined results. That's definitely not what you
want.
Tests are NOT a specification. I can't figure out from your
tests what the function is actually supposed to do. Good
programming starts with a specification. Without that, you're
sunk before you've started.
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.
OK. Since you labeled on argument charsPerLine, what's wrong
with labeling the next linesPerPage? (I'd probably have used
lineLength and pageLength, but it doesn't matter.) And youre
out argument "pageVector"? (A specification doesn't have to be
pure English text.)
That's really, of course, two problems, and should be done in
two separate functions. First break up the input into lines,
then break up the list of lines into pages. For that, I rather
prefer maintaining the lines as entries in a vector, so that I
don't have to scan for '\n' characters. Which means that
formatText would be something like:
void
formatText(
std::string const& in,
int lineLength,
int pageLength,
std::vector< std::string >&
pageVector )
{
std::vector< std::string >
lineVector ;
horizontalFormat( in, lineLength, lineVector ) ;
std::vector< std::string >::const_iterator
it = lineVector.begin() ;
while ( it != lineVector.end() ) {
std::string page ;
for ( int i = std::min( pageLength, lineVector.end() -
it ) ;
i > 0 ;
-- i ) {
page += *it + '\n' ;
++ it ;
}
pageVector.push_back( page ) ;
}
}
(That's just off the top of my head, without testing, so it
probably contains a few errors.)
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.
OK. That's about what I was looking for.
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.
The cyclomatic complexity is O(n), I'm pretty sure.
The basic principle is simple: you write a class which takes a
string in the constructor, and returns a single word each time a
function (getWord()) is called. You instantiate an instance of
that class at the top of the horizontalFormat function, using
the string "in", and loop calling getWord. Something like:
void
horizontalFormat(
std::string const& in,
int lineLength,
std::vector< std::string >&
lineVector )
{
WordGetter words( in ) ;
std::string line ;
for ( WordGetter::const_iterator it = words.begin() ;
it != words.end() ;
++ it ) {
std::string word( *it ) ;
int appendLength = word.size() ;
if ( line.size() != 0 ) {
++ appendLength ; // count space
}
if ( line.size() + appendLength <= lineLength ) {
if ( line.size() != 0 ) {
line += ' ' ;
}
line += word ;
} else {
if ( line.size() != 0 ) {
lineVector.push_back( line ) ;
}
line.clear() ;
if ( word.size() <= lineLength ) {
line += word ;
} else {
// additional line break logic needed here.
}
}
}
}
Writing the iterator for WordGetter, of course, is the tricky
part---the STL iterators are not really well designed for this,
and you may want to use a real iterator (like the one in GoF,
for example) instead. Easier to use and easier to implement.
--
James Kanze (GABI Software) email:james.kanze@gmail.com
Conseils en informatique orient=E9e objet/
Beratung in objektorientierter Datenverarbeitung
9 place S=E9mard, 78210 St.-Cyr-l'=C9cole, France, +33 (0)1 30 23 00 34