trjhtr

This is a question from the book "Cracking the Coding Interview".

Write a method to decide if two strings are anagrams or not

I think interviewer will not be convinced with this solution because here it is no test of logic. Please help me to optimise this solution.

#include <iostream>
#include <string>
#include <algorithm>
#include <cctype>

std::string toLower(std::string str)

 std::transform(str.begin(), str.end(), str.begin(),
 (unsigned char ch) return std::tolower(ch); );
 return str;


bool areAnagrams(std::string& str1, std::string& str2)


 str1 = toLower(str1);
 str2 = toLower(str2);
 std::sort(str1.begin(), str1.end());
 std::sort(str2.begin(), str2.end());

 if (str1.compare(str2) == 0)
 
 return true;
 
 else
 
 return false;
 


int main()

 std::string str1, str2;
 std::cout << "Enter String 1: ";
 std::getline(std::cin, str1);
 std::cout << "Enter String 2: ";
 std::getline(std::cin, str2);
 bool res = areAnagrams(str1, str2);
 if (res == 1)
 
 std::cout << "Strings are anagramn";
 
 else
 
 std::cout << "Strings are not anagramn";
 

I think interviewer will not be convinced with this solution because here it is no test of logic.

I'm not sure what you mean by "because here it is no test of logic," but if what you mean is "because I found this puzzle too easy," then good! That's what the interviewer is likely to be hiring for: people who solve problems easily and quickly.

When you get an easy problem in an interview, that may be a sign that the interviewer is trying to judge your coding skills rather than your algorithmic/research skills. So make sure your code is as polished as (reasonably) possible.

• areAnagrams takes its parameters by non-const reference. This is probably a bug. The interviewer will ask you to "explain your choice." Your answer should be something like "oops, I forgot to remove the &." (This reflects a little badly on your coding skills.)




• if (res == 1) is a very strange way to test for boolean true-ness. It would be less unusual, but still a minor yellow flag, to test for if (res == true) or if (res). It would be good to remove the useless variable and test directly for if (areAnagrams(str1, str2)).

Similarly, in the body of areAnagrams, you have written

if (str1.compare(str2) == 0)

 return true;

else

 return false;

This is a very long-winded and confusing way of writing

return str1 == str2;

and is kind of a big deal. The interviewer is not looking to hire people who write eight lines of convoluted code when one line of simple code would do the job.

You also have an extra blank line at the beginning of areAnagrams; this shows a possible tendency toward sloppiness. The interviewer is not looking to hire people who might "typo" their way into a bug.

On the plus side, your definition of toLower is very good! The interviewer might ask you to explain why you took by value instead of by const&. The interviewer might ask you whether the line return str; makes a copy of the string or whether the copy is elided. (Trick question! The answer is "neither.")

I see some things that may help you improve your code.

bool is not int

The bool type is a full-fledged first class type in C++. If I were an interviewer reading this code, I'd be perplexed:

bool res = areAnagrams(str1, str2);
if (res == 1)

 std::cout << "Strings are anagramn";

else

 std::cout << "Strings are not anagramn";

A similar thing is being done here:

if (str1.compare(str2) == 0)

 return true;

else

 return false;

First, we are comparing a bool to 1 (an int) which is odd enough. Next, if we're returning the result of the comparison, why don't we return the result of the comparison?

return !str1.compare(str2);

Better still:

return str1 == str2;

Understand references

The prototype of the toLower function is this:

std::string toLower(std::string str);
bool areAnagrams(std::string& str1, std::string& str2);

So reading this, the toLower makes a copy of its argument and areAnagrams uses references. However, the first few lines of the latter function are these:

str1 = toLower(str1);
str2 = toLower(str2);

There's little point to making copies and then assigning the copy back to the original. What I would recommend instead is to have toLower take a reference and areAnagrams pass by value. That way, we have a much more logical interface in which toLower modifies the passed string but areAnagrams does not.

Use auto to simplify code

The better choice for res in main would be to declare it auto instead of explicitly naming bool.

Consider the use of locale

Rather than writing your own toLower, why not use the one in <locale>? Here's how that might look:

auto& facetstd::use_facet<std::ctype<char>>(std::locale());
facet.tolower(&str1.front(), &str1.back());
facet.tolower(&str2.front(), &str2.back());

Use better naming

The function toLower is a good name because (with the suggested change mentioned above) it says what it actually does. However, res is not a good name and it's not necessary to have a separate variable anyway. Instead of this strange construction:

bool res = areAnagrams(str1, str2);
if (res == 1)

 std::cout << "Strings are anagramn";

else

 std::cout << "Strings are not anagramn";

I would probably instead have written this:

std::cout << "Strings " << (areAnagrams(str1, str2) ? "are" : "are not")
 << " anagramsn";

Although, as @TobySpeight points out in a comment, better for the purposes of translating the string into another language would be to keep the strings intact. One way to do that:

std::cout << (areAnagrams(str1, str2) ? "Strings are anagramsn"
 : "Strings are not anagramsn");

Declare variables each on a separate line

Clarify variable declaration by declaring each one on a single line. See Guideline ES.10

Consider namespace or static

During an interview, I'd probably ask why you chose not to encapsulate the functions in a namespace, and why the functions are not static. There are arguments both ways for each of those; be aware of what they are and be able to explain and defend your choices.

You know, when deciding whether you have anagrams, you are generally only interested in alpha-numeric characters, the rest are disregarded.

Next, making extraneous copies is generally a bad idea. So, accept a constant view and don't make a copy.

When you accept a mutable reference, be sure that it's obvious from the function-name and arity that it's a mutable reference.

If you can, avoid external linkage. It promotes inlining and avoids unfortunate name-clashes.

Finally, a faster algorithm is normally preferable:

bool isAnagram(std::string_view a, std::string_view b) noexcept 
 unsigned counts[1ULL + (unsigned char)-1] = ;
 for (unsigned char c : a)
 ++counts[std::tolower(c)];
 for (unsigned char c : b)
 --counts[std::tolower(c)];
 for (std::size_t i = 0; i < std::size(counts); ++i)
 if (counts[i] && std::isalnum(i))
 return false;
 return true;