Don't compute strlen(word)
!
A call to strlen is O(n)
in time, where n
is the length of the string.
By calling this function you get overall time complexity comparable with a hash function that depends on all characters of the input. And so you loose the presumed performance gain of hashing only 3 letters.
Just terminate the loop if you happen to encounter the \0
character.
Use array for l1, l2, l3
You can simplify your code if you treat those 3 as an array.
Don't create unnecessary variables only to return them on the next line
Just return the expression.
It's probably just a matter of preference, personally I'd even strip the unnecessary parenthesis. Seems much more readable, basically because there is much less to read...
Use the pointer as iterator
You get some small performance gain if you iterate over the word
pointer rather then introducing integer iterator i
.
Guard against NULL pointer
It may be wise to handle the case when word
is NULL to avoid possible segmentation fault.
Overuse of comments
You don't have to comment on every line of your code. It creates disturbance and gets out of date easily.
Describe all important about the function in the comment above the function.
Add comments inside the implementation only when "why" is not obvious. The "how" is never needed in the comment because that's what you discover by reading the code.
For example here
// Get index postion of word by substract it with ASCII Code
l2 = toupper(word[i]) - 'A';
The comment says the same as what the code does. If I were to write a comment for this line I would rather explain WHY toupper needs to be used there. But the info belongs to the main comment anyway...
Modified code - Putting it all together:
hash.c
#include <ctype.h>
#include "hash.h"
#define MAX_HASHED_LETTERS 3
unsigned int hash(const char *word)
{
if (word == NULL) {
return 0;
}
unsigned int L[MAX_HASHED_LETTERS] = {0, 0, 0};
unsigned int letter_index = 0;
for (char c = *word; c != '\0'; c = *(++word))
{
if (isalpha(c))
{
L[letter_index++] = toupper(c) - 'A';
if (letter_index == MAX_HASHED_LETTERS) {
break;
}
}
}
return L[0] * 676 + L[1] * 26 + L[2];
}
The macro MAX_HASHED_LETTERS
is there to improve readability, but it should be private to the hash
function. And so I placed it into the module hash.c
while a forward declaration of the function should be placed in header file hash.h
.
hash.h
#pragma once
/**
* Returns a hash of the word's first up to 3 "isalpha" characters.
* The function is case-insensitive meaning the returned value is the same for any case variation of the input.
* It returns 0 for NULL pointer, words that don't contain "isalpha" characters and words whose first up to 3 letters are all "a" or "A".
*
* Example: ....
*/
unsigned int hash(const char *word);
Generalize
In case it is needed we can generalize to be able to hash any number of first letters. We just need to get rid of the fixed size array L
and compute the hash incrementally. Although this will probably overflow for larger n (which may not be a problem though).
#define ALPHABET_SIZE ('Z' - 'A' + 1)
unsigned int hash_first_n_letters_ci(const char *word, unsigned int n)
{
if (word == NULL) {
return 0;
}
unsigned int letter_index = 0;
unsigned int hash = 0;
for (char c = *word; c != '\0'; c = *(++word))
{
if (isalpha(c))
{
hash = hash * ALPHABET_SIZE + (toupper(c) - 'A');
if (++letter_index == n) {
// skip the finalizing loop as all n letters were found and so the hash is already final
return hash;
}
}
}
// finalize the hash if less then n letters were found
for (; letter_index < n; ++letter_index) {
hash *= ALPHABET_SIZE;
}
return hash;
}
unsigned int hash_first_3_letters_ci(const char *word)
{
return hash_first_n_letters_ci(word, 3);
}
Test your code
It is good to add some tests to make sure everything works during refactoring. Actually it would be even better to write tests first. Let them fail. Then keep writing the implementation until all the tests succeed. Then improve the implementation while being sure everything still works.
A simple test suite can be carried out using assert
.
#include <assert.h>
int main()
{
assert(hash_first_3_letters_ci(NULL) == 0);
assert(hash_first_3_letters_ci("") == 0);
assert(hash_first_3_letters_ci("a") == 0);
assert(hash_first_3_letters_ci("aa") == 0);
assert(hash_first_3_letters_ci("aaa") == 0);
assert(hash_first_3_letters_ci("aaabcde") == 0);
assert(hash_first_3_letters_ci("'a") == 0);
assert(hash_first_3_letters_ci("'aa") == 0);
assert(hash_first_3_letters_ci("'aaa") == 0);
assert(hash_first_3_letters_ci("'aaabcde") == 0);
assert(hash_first_3_letters_ci("A") == 0);
assert(hash_first_3_letters_ci("AA") == 0);
assert(hash_first_3_letters_ci("AAA") == 0);
assert(hash_first_3_letters_ci("AAABCDE") == 0);
assert(hash_first_3_letters_ci("'A") == 0);
assert(hash_first_3_letters_ci("'AA") == 0);
assert(hash_first_3_letters_ci("'AAA") == 0);
assert(hash_first_3_letters_ci("'AAABCDE") == 0);
assert(hash_first_3_letters_ci("aab") == 1);
assert(hash_first_3_letters_ci("AAB") == 1);
assert(hash_first_3_letters_ci("aabbbbb") == 1);
assert(hash_first_3_letters_ci("AABBBBB") == 1);
assert(hash_first_3_letters_ci("'aab") == 1);
assert(hash_first_3_letters_ci("'AAB") == 1);
assert(hash_first_3_letters_ci("'aabbbbb") == 1);
assert(hash_first_3_letters_ci("'AABBBBB") == 1);
assert(hash_first_3_letters_ci("aba") == 26);
assert(hash_first_3_letters_ci("ababcd") == 26);
assert(hash_first_3_letters_ci("ABA") == 26);
assert(hash_first_3_letters_ci("ABABCD") == 26);
assert(hash_first_3_letters_ci("a'ba") == 26);
assert(hash_first_3_letters_ci("a'babcd") == 26);
assert(hash_first_3_letters_ci("A'BA") == 26);
assert(hash_first_3_letters_ci("A'BABCD") == 26);
assert(hash_first_3_letters_ci("b") == 676);
assert(hash_first_3_letters_ci("B") == 676);
assert(hash_first_3_letters_ci("ba") == 676);
assert(hash_first_3_letters_ci("BA") == 676);
assert(hash_first_3_letters_ci("baa") == 676);
assert(hash_first_3_letters_ci("baabcd") == 676);
assert(hash_first_3_letters_ci("BAA") == 676);
assert(hash_first_3_letters_ci("BAABCD") == 676);
assert(hash_first_3_letters_ci("ba'a") == 676);
assert(hash_first_3_letters_ci("ba'abcd") == 676);
assert(hash_first_3_letters_ci("BA'A") == 676);
assert(hash_first_3_letters_ci("BA'ABCD") == 676);
assert(hash_first_3_letters_ci("aaz") == 25);
assert(hash_first_3_letters_ci("aazzzz") == 25);
assert(hash_first_3_letters_ci("AAZ") == 25);
assert(hash_first_3_letters_ci("AAZZZZ") == 25);
assert(hash_first_3_letters_ci("azz") == 675);
assert(hash_first_3_letters_ci("azzzzz") == 675);
assert(hash_first_3_letters_ci("AZZ") == 675);
assert(hash_first_3_letters_ci("AZZZZZ") == 675);
assert(hash_first_3_letters_ci("zaa") == 676 * 25);
assert(hash_first_3_letters_ci("zaazyx") == 676 * 25);
assert(hash_first_3_letters_ci("ZAA") == 676 * 25);
assert(hash_first_3_letters_ci("ZAAZYX") == 676 * 25);
assert(hash_first_3_letters_ci("zzz") == (676 + 26 + 1) * 25);
assert(hash_first_3_letters_ci("zzzzyx") == (676 + 26 + 1) * 25);
assert(hash_first_3_letters_ci("ZZZ") == (676 + 26 + 1) * 25);
assert(hash_first_3_letters_ci("ZZZZYX") == (676 + 26 + 1) * 25);
return 0;
}
```