Programming Forums
Page 1 of 2 1 2 >
Show 40 post(s) from this thread on one page

Programming Forums (http://www.programmingforums.org/forumindex.php)
-   C (http://www.programmingforums.org/forum60.html)
-   -   Reducing Repetitive Code (http://www.programmingforums.org/showthread.php?t=11803)

Sane Nov 4th, 2006 7:01 PM
Reducing Repetitive Code
 
I'm trying to figure out a way to reduce the repetitiveness of this code. I see a couple options: load an array of all the cases from a simplified data file, or declare an array of all the cases.

I could change it to a switch, but I don't feel like having 50 break statements clogging up my source.

The code converts an integer that represents the event of an asynchronous key's state, to a string representing the event's signal in human-readable form. Only the relevant signals are being handled, nothing like mouse clicks, F keys, arrows, etc...

There is no coreleation between the number and the ASCII value for those characters, only to the extent of how it is defined by the Windows operating system. If only there was a function that could do this conversion for me. Any ideas?

:
  1. boolean getShift () {
  2.         return (GetAsyncKeyState (160) || GetAsyncKeyState (161));
  3. }
  4.  
  5. boolean getCase () {
  6.         return (GetKeyState (20) & 1 ? getShift () == false : getShift () == true);
  7. }
  8.  
  9. void keyToString (unsigned char key, char *result) {         
  10.         boolean isUpper = getCase ();
  11.  
  12.         result[0] = 0;
  13.  
  14.         if (key >= 'A' && key <= 'Z') {
  15.                 result[0] = key + (isUpper ? 0 : 32);
  16.                 result[1] = 0;
  17.         }
  18.         else if (key >= 96 && key <= 105) {
  19.                 result[0] = key-48;
  20.                 result[1] = 0;
  21.         }
  22.         else if (key == 8) {
  23.                 strcpy (result, "^backspace^");
  24.         }
  25.         else if (key == 9) {
  26.                 strcpy (result, "^tab^");
  27.         }
  28.         else if (key == 13) {
  29.                 strcpy (result, "\n");
  30.         }
  31.         else if (key == 17) {
  32.                 strcpy (result, "^ctrl^");
  33.         }
  34.         else if (key == 18) {
  35.                 strcpy (result, "^alt^");
  36.         }
  37.         else if (key == 32) {
  38.                 strcpy (result, " ");
  39.         }
  40.         else if (key == 46) {
  41.                 strcpy (result, "^delete^");
  42.         }
  43.         else if (key == 48) {
  44.                 strcpy (result, isUpper ? ")" : "0");
  45.         }
  46.         else if (key == 49) {
  47.                 strcpy (result, isUpper ? "!" : "1");
  48.         }
  49.         else if (key == 50) {
  50.                 strcpy (result, isUpper ? "@" : "2");
  51.         } 
  52.         else if (key == 51) {
  53.                 strcpy (result, isUpper ? "#" : "3");
  54.         }
  55.         else if (key == 52) {
  56.                 strcpy (result, isUpper ? "$" : "4");
  57.         }
  58.         else if (key == 53) {
  59.                 strcpy (result, isUpper ? "%" : "5");
  60.         }
  61.         else if (key == 54) {
  62.                 strcpy (result, isUpper ? "^" : "6");
  63.         }
  64.         else if (key == 55) {
  65.                 strcpy (result, isUpper ? "&" : "7");
  66.         }
  67.         else if (key == 56) {
  68.                 strcpy (result, isUpper ? "*" : "8");
  69.         }
  70.         else if (key == 57) {
  71.                 strcpy (result, isUpper ? "(" : "9");
  72.         }     
  73.         else if (key == 186) {
  74.                 strcpy (result, isUpper ? ":" : ";");
  75.         }
  76.         else if (key == 187) {
  77.                 strcpy (result, isUpper ? "+" : "=");
  78.         }
  79.         else if (key == 188) {
  80.                 strcpy (result, isUpper ? "<" : ",");
  81.         }
  82.         else if (key == 189) {
  83.                 strcpy (result, isUpper ? "_" : "-");
  84.         }
  85.         else if (key == 190) {
  86.                 strcpy (result, isUpper ? ">" : ".");
  87.         }
  88.         else if (key == 191) {
  89.                 strcpy (result, isUpper ? "?" : "/");
  90.         }
  91.         else if (key == 192) {
  92.                 strcpy (result, isUpper ? "~" : "`");
  93.         }
  94.         else if (key == 219) {
  95.                 strcpy (result, isUpper ? "{" : "[");
  96.         }
  97.         else if (key == 220) {
  98.                 strcpy (result, isUpper ? "|" : "\\");
  99.         }
  100.         else if (key == 221) {
  101.                 strcpy (result, isUpper ? "}" : "]");
  102.         }
  103.         else if (key == 222) {
  104.                 strcpy (result, isUpper ? "\"" : "'");
  105.         }
  106. }

grumpy Nov 4th, 2006 9:56 PM
I haven't tested this with a compiler, but this should be enough to get you started.

Basic notion is that you have some ranges that you are mapping, and finding a technique to map anything in that range.
:
boolean getShift () {
        return (GetAsyncKeyState (160) || GetAsyncKeyState (161));
}

boolean getCase () {
        return (GetKeyState (20) & 1 ? !getShift () : getShift ());
}

char ExtractChar(unsigned char key, int lower, int upper, const char *data)
{
    /* return mapped result, or zero if we can't do the mapping */

    return (key >= lower && key <= upper) ? data[key - lower]: '\0';
}

void keyToString (unsigned char key, char *result) {       
        boolean isUpper = getCase ();
   
        if (key >= 96 && key <= 105) {
                result[0] = key-48;
                result[1] = 0;
        }
        else if (key == 8) {
                strcpy (result, "^backspace^");
        }
        else if (key == 9) {
                strcpy (result, "^tab^");
        }
        else if (key == 13) {
                strcpy (result, "\n");
        }
        else if (key == 17) {
                strcpy (result, "^ctrl^");
        }
        else if (key == 18) {
                strcpy (result, "^alt^");
        }
        else if (key == 32) {
                strcpy (result, " ");
        }
        else if (key == 46) {
                strcpy (result, "^delete^");
        }
        else {
                result[1] = 0;
                if (!isUpper) {
                        if (key >= 'A' && key <= 'Z')
                              result[0] = key + 32;
                        else
                              result[0] = ExtractChar(key, 48, 57, "0123456789") ||
                                          ExtractChar(key, 186, 192, ";=,-./`") ||
                                          ExtractChar(key, 219, 222, "[\\]'");
                }
                else if (isUpper) {
                          if (key >= 'A' && key <= 'Z')
                                result[0] = key;
                          else
                                result[0] = ExtractChar(key, 48, 57, ")!@#$%^&*(") ||
                                            ExtractCha(key, 186, 192, ":+<_>?~") ||
                                            Extract(key, 219, 222, "{|}\"");
                }
      }
}

Sane Nov 4th, 2006 10:15 PM
Ahh, yes. That's a good idea! I was trying to think of someway to use strings of data. This way looks perfect. I'll take your idea and come up with my own code. Thanks.

2roll4life7 Nov 7th, 2006 12:47 PM
Quote:
Originally Posted by grumpy (Post 118364)
I haven't tested this with a compiler, but this should be enough to get you started.

Basic notion is that you have some ranges that you are mapping, and finding a technique to map anything in that range.
:
boolean getShift () {
        return (GetAsyncKeyState (160) || GetAsyncKeyState (161));
}

boolean getCase () {
        return (GetKeyState (20) & 1 ? !getShift () : getShift ());
}

char ExtractChar(unsigned char key, int lower, int upper, const char *data)
{
    /* return mapped result, or zero if we can't do the mapping */

    return (key >= lower && key <= upper) ? data[key - lower]: '\0';
}

void keyToString (unsigned char key, char *result) {       
        boolean isUpper = getCase ();
   
        if (key >= 96 && key <= 105) {
                result[0] = key-48;
                result[1] = 0;
        }
        else if (key == 8) {
                strcpy (result, "^backspace^");
        }
        else if (key == 9) {
                strcpy (result, "^tab^");
        }
        else if (key == 13) {
                strcpy (result, "\n");
        }
        else if (key == 17) {
                strcpy (result, "^ctrl^");
        }
        else if (key == 18) {
                strcpy (result, "^alt^");
        }
        else if (key == 32) {
                strcpy (result, " ");
        }
        else if (key == 46) {
                strcpy (result, "^delete^");
        }
        else {
                result[1] = 0;
                if (!isUpper) {
                        if (key >= 'A' && key <= 'Z')
                              result[0] = key + 32;
                        else
                              result[0] = ExtractChar(key, 48, 57, "0123456789") ||
                                          ExtractChar(key, 186, 192, ";=,-./`") ||
                                          ExtractChar(key, 219, 222, "[\\]'");
                }
                else if (isUpper) {
                          if (key >= 'A' && key <= 'Z')
                                result[0] = key;
                          else
                                result[0] = ExtractChar(key, 48, 57, ")!@#$%^&*(") ||
                                            ExtractCha(key, 186, 192, ":+<_>?~") ||
                                            Extract(key, 219, 222, "{|}\"");
                }
      }
}
Nice idea, it'd probably look a little neater with a switch though, IMO.

grumpy Nov 9th, 2006 4:43 AM
Quote:
Originally Posted by 2roll4life7 (Post 118565)
Nice idea, it'd probably look a little neater with a switch though, IMO.
Depends on your definition of "neat", I guess. Given that some of the conditions involve the value of "key" being in a range, that means a switch/case approach must list all the cases in that range. That can easily become unwieldy and difficult to maintain (eg it is easy to leave out one of the cases and that is hard to pick up without quite comprehensive testing).

Polyphemus_ Nov 9th, 2006 1:06 PM
Quote:
Originally Posted by grumpy (Post 118661)
Depends on your definition of "neat", I guess. Given that some of the conditions involve the value of "key" being in a range, that means a switch/case approach must list all the cases in that range. That can easily become unwieldy and difficult to maintain (eg it is easy to leave out one of the cases and that is hard to pick up without quite comprehensive testing).
You could create an if block for the ranges, and put the single keys in a switch. Not sure if that would look neater, though.

grumpy Nov 9th, 2006 4:54 PM
2Roll4Life7 and Polyphemus_, if you want to play with getting it "neater"

1) Define what you mean by "neatness", and provide some measure by which you determine how neat some code is;

2) Try different approaches.

In keeping on topic with this thread, one measure might be related to the amount of repetitive code ......

In practice, I don't tend to like nesting switch/case blocks in if/else blocks (or vice versa) because it takes more time/effort to read and understand, which means it takes more time/effort to maintain.

Dizzutch Nov 9th, 2006 5:21 PM
There is actually a term for the act of cleaning these kind of things up, it's called "Refactoring. As previously stated, a lot of it is just identifying what code could go into a function. Don't be afraid of having functions with long signatures. A common refactoring process is also creating large arrays with all the necessary information, sometimes even including function names and parameters.
example: if your program looks like this
:
function bla
{
    if a then 1
    if b then 2
    if c then 3
    if d then 4
}
you could make an array
:
{
( a, 1 )
( b, 2 )
( b, 3 )
( c, 4)
}
and replace the top code with something like this
:
foreach $i in $array {
  if $i[0] then $i[1]
}
where $i[1] is actually an entire conditional and $i[2] is actually the name of a subroutine that can be parsed with some regex and used.

the string "subrouting:a:b:c"
could be parsed with
:
/^subroutine:(.*):(.*):(.*)/
and called run as
:
subroutine(a, b, c)
you can match the name of the subroutine to call the correct one in the event you need to call more than one.

now in some languages this is not as easy as in others (PERL is very good at things like this for example).
Excuse me for my rant on Refactoring, but it's a good skill to have, and I use it everyday for my job and on my personal projects. Eventually one can get good at it that the code written on the first try does not have to be refactored anymore, but don't be afraid to write it out the long way and look back afterwards.

Good Luck,

-Dizz

Polyphemus_ Nov 10th, 2006 2:53 AM
Dizz! You're back!

2roll4life7 Nov 11th, 2006 12:38 AM
Well, what I meant was by putting all the numbers into a switch statement and then in the default you can have just a couple of if statments for the ranges. Or alternatively you could have listed all the numbers you wanted for a range with no break statements between them and have your code for that range after that. I'm not too crazy about either of those though. As Grumpy said, it depends on your definition of neat.

Those were just my observations from glancing at the code... If I were faced with this problem myself, I'd probably just do a lookup in a hard coded hash table (this doesn't reduce repetitive code, but I just think it's a better approach [assuming you only want a few selected key codes and not all]). Then again if we are stuck stictly with C, that would require writing your own hash table implementation or finding a decent one by somebody else. Perl could probably do this in 2 lines, lol, and even C++ would give you the option of the STL hash_set.

If you wanted to just map all the key codes with characters, you could do it with a large array of strings
eg:
:
char *keycodes[] = {"VK_LBUTTON", "VK_RBUTTON ", "VK_CANCEL", "VK_MBUTTON ", "VK_XBUTTON1", "VK_XBUTTON2", NULL, "VK_BACK", "VK_TAB", "...etc..."};
Then instead of branching off to a function, you can just index into the array with the key and it'll give you the corresponding value. (you can even just put NULL if you don't want to map a specific one or two, but it must keep the proper indices.)

So both solutions I described don't actually reduce the code, but that doesn't mean they aren't good solutions. Infact, a hardcoded lookup table or an index into an array are both faster than branching into a function that performs calculations. Also, if you plan on maintaining this code, I'd go with whatever way you're more comfortable with.

If you'd like me to elaborate more, I won't mind, just ask.

* My example line from above was using the keycodes from this list: http://msdn.microsoft.com/library/de...alKeyCodes.asp


All times are GMT -5. The time now is 1:26 AM.
Page 1 of 2 1 2 >
Show 40 post(s) from this thread on one page

Powered by vBulletin® Version 3.7.0, Copyright ©2000 - 2008, Jelsoft Enterprises Ltd.
Copyright ©2007 DaniWeb® LLC