0
\$\begingroup\$

I have a section of code that repeats many lines and I am looking to refactor this code possibly so that a function can be used instead of the same lines of code each time. Below is the section of code and the repeating code occurs in each if statement

Traversal::Traversal(string source, bool isFileName)
 {
      using namespace XML_Parser;
      using std::endl;

      std::ostringstream ppd;
  if (isFileName)
  {
      string content, line;
      std::ifstream file(source);
      if (! file)
      {
          ppd << endl << "error opening file " << source;
          cRep = ppd.str();
          cSuc = false;
          return;
      }
      ppd << endl << " file " << source << " opened ";
      while (file.good())
      {
          getline(file, line);
          content += '\n';
          content += line;
      }
      ppd << endl << "file " << source << " read ";
      source = content;
  }
  if (! elementExists(source,"afcd"))
  {
      ppd << endl << "no afcd tag";
      cRep = ppd.str();
      cSuc = false;
      return;
  }
  if (! elementExists(source,"rteirl"))
  {
      ppd << endl << "no rteirl tag";
      cRep = ppd.str();
      cSuc = false;
      return;
  }

How would I go about making this cleaner?

\$\endgroup\$
1
  • 2
    \$\begingroup\$ while (file.good()) is wrong for the same reasons that while (!file.eof()) is. \$\endgroup\$
    – molbdnilo
    Commented Feb 17, 2017 at 10:29

3 Answers 3

1
\$\begingroup\$

The basic aim is to put repeating parts in the function, while passing as a parameter anything that's different:

if (elementMissing(source, "afcd")) {
    return;
}

if (elementMissing(source, "rteirl")) {
    return;
}

bool elementMissing(source, element) {
    if (! elementExists(source, element)) {
        ppd << endl << "no " << element << " tag";
        cRep = ppd.str();
        cSuc = false;
        return true;
    } else {
        return false;
    }
}

I'm not sure what cRep and cSuc are supposed to be doing, so I can't help you with that part.

\$\endgroup\$
0
\$\begingroup\$

Use a helper function, something along the line of:

bool Helper(std::string tagtype)
{
  if (! elementExists(source, tagtype))
  {
      ppd << endl << "no " << tagtype << " tag";
      cRep = ppd.str();
      cSuc = false;
      return true;
  }
  return false;
}

obviously you need to figure out how to pass in (by reference) the values of ppd, cRep, etc.

\$\endgroup\$
0
\$\begingroup\$

How about:

void
Traversal::BadStatus( std::ostringstream& ppd, const string& msg )
{
  ppd << endl << msg;
  cRep = ppd.str();
  cSuc = false;
}

Then:

if( ! elementExists(source, "asdf") ) {
    BadStatus("no asdf tag");
    return;
}

Usually, though, the best solution to an error at construction time is to throw an exception.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.