My IsIsogram() function

So here is my take on building the IsIsogram() function. I have a slight variation on the IsIsogram method. I have my loop outside of the IsIsogram() function so it is only checking the TMap (which I declare in the header file) to see if the key exists.

The reason I did this is the guess should also be checked to see if it has valid characters (they should only be letters of the alphabet). So instead of looping through the whole guess again to check if the guess is an alphabetical characters, I do both checks in a single loop.

Though, one thing to note, is that if I had IsIsogram() function with a const after it, I would get an error (I kept the error message in my first screenshot). When I don’t use const my code runs fine, but if I do I end up getting an error. I believe it has something to do with a type mismatch between a const character and the type of the key of the TMap. If anyone would be able to explain in more detail (or tell me I’m completely wrong) I would love to know!

As your LetterCheck is a class member variable, you cannot modify it from within a const method. You’d be able if it were declared as mutable as mutable class member variables can be changed by const methods. Or better yet, declare LetterCheck as local variable within IsIsogram method instead of having it as a class member.

Thank you for the reply I really do appreciate it. I did find a way to make it constant, I just have to pass my TMap variable into the IsIsogram() function I have. I’m still a little confused about why it didn’t let me just look in the TMap variable (so any additional explanation would be greatly appreciated).

With the way my code functions the function IsIsogram() isn’t updating the TMap but is instead looking to see if the key exists then returning true or false. When it does this check to see if the key exists, is that technically a modification to the TMap variable? I am setting a Key and Value outside of the IsIsogram function (in my check valid selection). Also when I pass the TMap variable in the function it can be constant.

In response to declaring the variable into the IsIsogram() function, I am unable to due to one key difference in my code vs how the code was written in the lessons. I am doing all of the checks that require looping through the word in one loop. So my ValidGuess function loops through the word itself, then calls each of the checks we have to do in that loop (is it an isogram, are the characters all alphabetical, are the characters all lower case). So if I declare the TMap variable in my IsIsogram() function it would only map the 1 letter it was looking at then the TMap would be discarded.

The reason I chose to do it this way is my current understanding of O(n) notation. If I understand it correctly, the process that I am using is currently O(n) (one loop that checks the guess is all alphabetical characters and is not an isogram) where the same process with the design in the lectures would take O(m*n) (Where m is the number of separate times they loop through the word). Currently m is 2 in my example (1 loop to check the word is an isogram and 1 loop to check characters are alphabetical) but that would increase with every check that you ran on the word. If I add in the lowercase check my code stays at O(n) vs the lecture code goes to O(3n).

{If I am incorrect about this someone please correct me, this is just my current understanding and it may be wrong}

It’s because std::map::operator [] is a non const function - it has a side effect that is not visible at first glance which is if the key is not present, it will insert the key so it will become present.
So your IsIsogram function, would potentially modify LetterCheck map if Letter hadn’t been added into the LetterCheck previously. Adding LetterCheck to IsIsogram as paramter will work. However if you want to skip this but still have your code structure like you have it, you can use std::map::find instead of the [] operator. So your const IsIsogram would be like this:

bool fBullCowGame::IsIsogram(char Letter) const {
  if (LetterCheck.find(Letter) != LetterCheck.cend()) {
    return false;
  return true;
  // A shorter way to code this:
  // return LetterCheck.find(Letter) != LetterCheck.cend();

You’re right that your code is more effective having one for loop instead of couple more etc. However note that O(3*n) is still O(n). All in all while in practice it may not be worth optimizing to this extent, it is still a good code and good learning experience I think. So good job there.

That makes a lot more sense now. Thank you very much! I appreciate you taking the time to explain it all thoroughly, and writing code showing how it works.

Privacy & Terms