principlecsharpMinor
Compare the index of each char in a string in an alphabet array to a range of numbers
Viewed 0 times
alphabettheeacharraycharrangenumbersindexcomparestring
Problem
I saw this codegolf challenge and I set out to try and write a solution for it. I'm nowhere near an expert codegolfer (or programmer), but it was an interesting exercise.
Now I'm wondering how to improve my code, as it feels really bulky (especially compared to some of the answers to the challenge). Note that I'm not looking for ways to golf this code, I'm merely looking for general improvements and optimizations, hints and tips.
I hope the title is clear as to what the program does, it was pretty hard to describe!
Anyway, what my program does is pretty simple:
My code:
Now I'm wondering how to improve my code, as it feels really bulky (especially compared to some of the answers to the challenge). Note that I'm not looking for ways to golf this code, I'm merely looking for general improvements and optimizations, hints and tips.
I hope the title is clear as to what the program does, it was pretty hard to describe!
Anyway, what my program does is pretty simple:
- It accepts a string as input
- It then fetches the index of each char in the string from the array
- It checks if that char is within a certain range
- Each range corresponds with a classic mobile phone button (0,1,2 = A,B,C)
- The current button is compared to the previously used button
- If the buttons match, the string does not pass and it returns false
My code:
int previousButton = -1;
char[] letters =
{
'a', 'b', 'c', // 2
'd', 'e', 'f', // 3
'g', 'h', 'i', // 4
'j', 'k', 'l', // 5
'm', 'n', 'o', // 6
'p', 'q', 'r', 's', // 7
't', 'u', 'v', // 8
'w', 'x', 'y', 'z', // 9
' ' // 0
};
for (int i = 0; i = 0 && index = 3 && index = 6 && index = 9 && index = 12 && index = 15 && index = 19 && index = 22 && index <= 25) {
currentButton = 9;
} else if (index == 26) {
currentButton = 0;
}
if (previousButton == currentButton) {
return false;
}
previousButton = currentButton;
}
return true;Solution
There are two alternatives I can recommend, one alternative avoids all the
The second alternative is to trade code space, for memory space.
First though, improving your current solution...
Current version
Your current version has a lot of unnecessary conditions in the cascading if/else system.
If you check for 'low' values first, then you can assume the next value's lower range is already handled. It is easier to explain this by example.... you have:
But this will produce the same results, with half the comparisons:
Note how I have also used the conventional C# style there for the braces....
A switch statement can work on chars:
The above code (included in a function) will encode each char to a key, and unmapped chars will return 1.
A second common way to do this is to prepopulate an array with the indexes for each char:
Then use that array as a simple lookup:
That converts the char to an offset in the keys array. I have taken the liberty of coding this up in Ideone to ensure it works (which it does, except for the handling of space characters)
if/else/if cascading, and replaces it with a single 'switch' statement. Switch statements are optimized at compile time so that, effectively, each char/operation takes as long as any other. it makes sense to extract that to a function too.The second alternative is to trade code space, for memory space.
First though, improving your current solution...
Current version
Your current version has a lot of unnecessary conditions in the cascading if/else system.
If you check for 'low' values first, then you can assume the next value's lower range is already handled. It is easier to explain this by example.... you have:
if (index >= 0 && index = 3 && index = 6 && index <= 8) {But this will produce the same results, with half the comparisons:
if (index < 0)
{
currentButton = 1; //invalid index, char not found?
}
else if (index <= 2)
{
currentButton = 2;
}
else if (index <= 5)
{
currentButton = 3;
}
else if (index <= 8)
{Note how I have also used the conventional C# style there for the braces....
- Switch:
A switch statement can work on chars:
switch (currentLetter)
{
case 'a':
case 'b':
case 'c':
return 2;
case 'd':
case 'e':
case 'f':
return 3;
.....
default:
return 1; // whatever you need for an invalid char
}The above code (included in a function) will encode each char to a key, and unmapped chars will return 1.
- In memory lookup
A second common way to do this is to prepopulate an array with the indexes for each char:
int[] keys = new int[]{2,2,2,3,3,3,4,4,4,5,5,5,6,6,6,7,7,7,7,8,8,8,9,9,9,9};Then use that array as a simple lookup:
if (currentChar 'z')
{
return 1;
}
return keys[currentChar - 'a'];That converts the char to an offset in the keys array. I have taken the liberty of coding this up in Ideone to ensure it works (which it does, except for the handling of space characters)
Code Snippets
if (index >= 0 && index <= 2) {
currentButton = 2;
} else if (index >= 3 && index <= 5) {
currentButton = 3;
} else if (index >= 6 && index <= 8) {if (index < 0)
{
currentButton = 1; //invalid index, char not found?
}
else if (index <= 2)
{
currentButton = 2;
}
else if (index <= 5)
{
currentButton = 3;
}
else if (index <= 8)
{switch (currentLetter)
{
case 'a':
case 'b':
case 'c':
return 2;
case 'd':
case 'e':
case 'f':
return 3;
.....
default:
return 1; // whatever you need for an invalid char
}int[] keys = new int[]{2,2,2,3,3,3,4,4,4,5,5,5,6,6,6,7,7,7,7,8,8,8,9,9,9,9};if (currentChar < 'a' || currentChar > 'z')
{
return 1;
}
return keys[currentChar - 'a'];Context
StackExchange Code Review Q#62637, answer score: 8
Revisions (0)
No revisions yet.