The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

How to remove if loops

I my C++ Win32 program I have like 130 if loops in one function.

if(type == "ABC")
{
  target = "abcdefghijklmnopqrstuvwxyz";
}
....
...
....
<130 times>

Obviously looks ugly. I can replace that with a switch. But still it will be ugly.

Any innovative ideas.
type are string and target is char *
Tony Send private email
Friday, March 23, 2007
 
 
It looks like you could use an associative array.  I'm not familiar with how to do this in C++, but in perl:

my %hash = {

Friday, March 23, 2007
 
 
In earlier days (1978) this would be called a "Transaction Center".  A "Transaction Center" has to make a decision (or several decisions) based on its input.  And the number of possible decisions can be high.  So Transaction Centers can take up quite a few lines of code, even though they're doing something simple.

Typical solutions:
1.  Leave it as is.  Transaction Centers tend to be large, but simple.  So it can make 1 of 130 possible decisions, then go on.  This will be large no matter what you do.

2.  Refactor.  'Nest' your decisions, if at all possible.  If 'nesting' IS possible, this can make what's going on more clear.  If 'nesting' is not possible, this can make a confusing situation even harder to maintain.

3.  Convert to a 'lookup' table.  'Hash' your input characters into a value.  Use the HashValue as an index into a lookup table.  In the lookup table will be structures which contain both the input characters and some decision indicating value.  The input characters are so you can validate that the HashValue actually matches your input.
AllanL5
Friday, March 23, 2007
 
 
The comment parser raped me!!


It looks like you could use an associative array.  I'm not familiar with how to do this in C++, but in perl:

my %hash = {
    "ABC" => "abcdefghijklmnopqrstuvwxyz",
    "BCD" => "abcdefghijklmnopqrstuvwxyz",
    "CDE" => "abcdefghijklmnopqrstuvwxyz",
};

my $type = "ABC";
my $target = $hash{$type};
Justin
Friday, March 23, 2007
 
 
if is a statement, not a loop.

How about a massive ternary operator:

target =
  type == "ABC" ? "abcdefghijklmnopqrstuvwxyz" :
  type == "DEF" ? "defghi" :
  type == "GHI: ? "ghi :
  "undefined";

Does this list ever grow or change? If so, what about storing the mappings as XML and loading them from a config file?
Just to piss off the ternary haters Send private email
Friday, March 23, 2007
 
 
Wow, "Just to", you achieved your purpose.  I AM a "ternary" operator hater -- but I had no idea it could be used as you call out.

On the one hand, it's a really clever hack.  On the other, I really do hate the ternary operator, and I suspect your solution isn't very 'general' -- it only works to translate one short character string to another string.

But thanks for the lesson, that really is a clever hack.
AllanL5
Friday, March 23, 2007
 
 
On the other, other hand, I believe using the Ternary operator will require O(n) time to make the decision.  Where a hash table would take O(1) time.

For 130 entries, that might not be bad.
AllanL5
Friday, March 23, 2007
 
 
You can't do a switch with strings in C/C++.
This is what I do:

struct { const char* pSrc; const char* pDest; } aConv[] =
{
  "ABC", "abcdefghijklmnopqrstuvwxyz",
  "DEF", "defghi",
  "GHI", "ghi",
  NULL, NULL
};

int nItem = 0;
while ( aConv[nItem].pSrc )
{
  if ( aConv[nItem].pSrc == type )
  {
    target = aConv[nItem].pDest;
    break;
  }
}

Or even better, load it into a map:

while ( aConv[nItem].pSrc )
  map.SetAt( aConv[nItem].pSrc, aConv[nItem].pDest );

then simply:

map.Lookup( type, target );
Ben Bryant
Friday, March 23, 2007
 
 
and don't forget to increment, ++nItem :)
Ben Bryant
Friday, March 23, 2007
 
 
+1 to Ben Bryant's solution

I just refactored some code in a very similar way.  I also made the lookup table a class, did the map initialization in the constructor, and declared  a single instance of the lookup table class at file scope (static.) 

PS, This is from memory, there may be a bug or two.


#include <map>
#include <string>

class LookupTable
{
public:
  LookupTable()
  {
      // TODO:: Initialize the map...
  }

  const std::string& Find(const std::string& Key) const
  {
      static const std::string Empty("");
      stringmap::const_iterator i = table.find( Key );
      if ( i != table.end() )
      {
        i->second;
      }     
      return Empty;
  }
private:
  std::map<std::string,std::string> table;
};

// Declare on instance at file scope.
static LookupTable TheLookupTable;

// Reimplement legacy function to delegate to LUT.
const char* LegacyReplacement(const char* key)
{
  return TheLookupTable.Find( key ).c_str();
}
Meganonymous Rex Send private email
Friday, March 23, 2007
 
 
In Find(...) The line:

  i->second;

Should be replaced with
 
  return i->second;
Meganonymous Rex Send private email
Friday, March 23, 2007
 
 
Thanks guys. It helped.
Tony Send private email
Friday, March 23, 2007
 
 
When you don't want to use tables or hash maps, then a symmetric formatting of the code can help a lot, given that the lines don't become too long (with monospaced font):

if      (type == "ABC") { target = "abcdef..."; }
else if (type == "DEF") { target = "defghi";    }
else if (type == "GHI") { target = "ghi";      }
Secure
Saturday, March 24, 2007
 
 
sorry i got to this late. I skimmed the solutions and I would  not consider them to be very good refactoring solutions. The solutions seem to be better than the original code but they are still procedural progamming.

Anyways when you have alot of if then else or switch statements you usually need to  create a class with subclasses. When the code runs it polymorphically runs the method of the subclasses that encapuslates your logic.

There are alot of other posibilities depending on the context this is done in.
Michael
Sunday, March 25, 2007
 
 
oh yeh anytime you have a method that large there is serious oo refactoring needed
Michael
Sunday, March 25, 2007
 
 
Yes, so how exactly do you decide which sub-class object you are going to create based on user input?

With some sort of switch statement.  :)  So if this is appearing many times in the code, sure refactor.  If its just once, it doesn't get any better than that really.

Sunday, March 25, 2007
 
 
Ah, so Michael, you suggest 'breaking' this long 'decision' code into multiple sub-classes, one for each option, each with a constructor which stores the 'translation', and with a method which returns its 'translation'.

This could be quite simple -- have a 'base' class with 99% of the functionality, and one over-ridden method in each inheritor to return the correct value.

Then, you'll let the run-time use polymorphism to return the correct string.

Well, this would work, but I'd claim this would be even MORE verbose (take up more lines of code), and be HARDER to maintain (now you have 130 classes to maintain instead of one routine).

And in effect, instead of creating a simple 'hash', you're using the object hierarchy and the run-time environment to do the hash 'for you'.  And in this way you avoid "procedural code" and make a more "object oriented" solution.

Sorry, I can't agree.  And I do think there are even better "object oriented" approaches, like a Dictionary, or a Map, which would be less verbose.

But it now seems to me this is not a "procedural" versus "oo" argument.  Instead, this is a "bad oo" versus "better oo" argument.
AllanL5
Monday, March 26, 2007
 
 
Allan:  The point is, how do you convert the user input to the appropriate object.

Really the issue is, how are you going to convert from a runtime value to a static type.  In C++, this requires some kind of switching construct as above!

Monday, March 26, 2007
 
 
Agreed.
AllanL5
Monday, March 26, 2007
 
 
Let's be serious, Michael.  The problem is: we want to find a string in a table based on a string key value.  This is the *textbook* example of an appropriate use for a map/associative array.

Please enlighten the group as to an alternative that would be as clear as say, the one I proposed.

Or were you trolling?
Meganonymous Rex Send private email
Monday, March 26, 2007
 
 
>I skimmed the solutions and I would  not consider them to be very good refactoring solutions

funny! it is a simple hash table lookup; designing an object architecture around it would be a laughable misapplication of object oriented design.
Ben Bryant
Monday, March 26, 2007
 
 
if its logic and assignment: ternary operator or switch block

Monday, March 26, 2007
 
 
The only reasonable data structure for the lookup is a map or hash, as BenB and MegRex pointed out.  The business about if statements, switches, and -?-:-  is just nonsense. Good design separates data (in this case the associations) from logic.

Relatedly, what happens when case number 131 comes along?  Do you want to modify your code and recompile, and recompile all dependencies, etc?  No.  The associations should be moved out to a separate file, that is read in at run time to build the hash.

For implementation use std::map<std::string, std::string>.
Will Dowling Send private email
Monday, March 26, 2007
 
 
A map works for value => value translations.  Which is the question here.

If-tree / select works for value => value and value => type.

Type information needs to be compiled into the system. 

The problem of converting from a value => type requires the if\ select concept. 

If you want to make it so that you can add new cases without modifying old code, a template solution could potentially be used.  But its limited on the types of values it can accept.

Monday, March 26, 2007
 
 
This assumes that your lookup table will only change at compile time, just like your if tree.
The solution in C++ :

// Template trick for calculating array size.
template <typename T, int N>
char (&arraysize(T(&)[N]))[N];

// Static array used to initialize map used to convert type strings to target strings
const std::pair<std::string, std::string> MyNameSpace::initTypeToTarget[] = {
  std::pair<std::string, std::string> ( "ABC", "abcdefghijklmnopqrstuvwxyz" ),
  std::pair<std::string, std::string> ( "BCD", "abcdefghijklmnopqrstuvwxyz" ),
  std::pair<std::string, std::string> ( "CDE", "abcdefghijklmnopqrstuvwxyz" ),
  std::pair<std::string, std::string> ( "DEF", "abcdefghijklmnopqrstuvwxyz" ),
  std::pair<std::string, std::string> ( "EFG", "abcdefghijklmnopqrstuvwxyz" )
};

// Static map used to convert strings to strings
const std::map<std::string, std::string> MyNameSpace::mapTypeToTarget ( initTypeToTarget, initTypeToTarget + sizeof arraysize ( initTypeToTarget ) );

// Unsure of whether or not a type string will exist in the map
std::string MyNameSpace::TypeToTarget (std::string const& strTypeName)
{
    if (mapTypeToTarget.count(strTypeName))
    {
        return mapTypeToTarget.find(strTypeName)->second;
    }

    return "";
}

// Sure that a type string will always exist in the map
std::string MyNameSpace::TypeToTarget (std::string const& strTypeName)
{
    return mapTypeToTarget.find(strTypeName)->second;
}
DougM
Tuesday, April 03, 2007
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz