C++

Forum

 
Refactoring code, to make it cleaner

vaynex (11)
I have a section of code that i am looking to refactor as in each conditional statement it is repeating lines of code that is used in another conditional statement.

the repeated code is
1
2
3
 
 cRep = ppd.str();
      cSuc = false;
      return;



the full section i want to refactor is below:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
 
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;
      }



i've been looking into helper functions, how would i go about making that? thanks



Last edited on
JLBorges (13770)
1
2
3
4
5
 
void with_error( std::string msg )
{
    cRep = '\n' + msg ;
    cSuc = false;
}


and then:

1
2
3
4
5
6
7
 
if( !file ) return with_error( "error opening file " + source ) ;

// ...

if ( !elementExists(source,"afcd") ) return with_error( "no afcd tag" ) ;

// etc



...........................................................................

or, alternatively:

1
2
3
4
5
 
void with_error( std::ostringstream& stm, std::string msg )
{
    cRep = stm.str() + '\n' + msg ;
    cSuc = false;
}


and then: 
1
2
3
4
5
6
7
 
if( !file ) return with_error( ppd, "error opening file " + source ) ;

// ...

if ( !elementExists(source,"afcd") ) return with_error( ppd, "no afcd tag" ) ;

// etc



Edit: woo, just noticed that it is a constructor.

Factor the code in the constructor into a private function - say, void init(string source, bool isFileName) - and call that from the constructor.
Last edited on
dhayden (5798)
Another option, building on JLBorges's idea of an init() function:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
 
bool init2(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;
              return false;
          }
          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";
          return false;
      }
      if (! elementExists(source,"rteirl"))
      {
          ppd << endl << "no rteirl tag";
          return false;
      }
      return true;
}

void init(string source, bool isFileName) {
    if (!init2(source, isFileName)) {
        crep = ppd.str();
        csuc = false;
    }
}
Topic archived. No new replies allowed.