Hackerrank Inherited Code

Question

I'm back at it again with an Hackerrank challenge-Inherited Code . I just learnt exception and I thought it will be a great idea to get a second opinion on my implemented BadLengthException struct .

You inherited a piece of code that performs username validation for your company's website. The existing function works reasonably well, but it throws an exception when the username is too short. Upon review, you realize that nobody ever defined the exception.

The inherited code is provided for you in the locked section of your editor. Complete the code so that, when an exception is thrown, it prints Too short: N (where \$N\$ is the length of the given username).

Input Format

The first line contains an integer, \$T\$ , the number of test cases. Each of the \$T\$ subsequent lines describes a test case as a single username string, \$ U\$ .

Constraints

• \$ 1 \leq T \leq 1000\$
• \$ 1 \leq |U| \leq 100\$
• The username consists only of uppercase and lowercase letters.

Output Format

You are not responsible for directly printing anything to stdout. If your code is correct, the locked stub code in your editor will print either Valid (if the username is valid), Invalid (if the username is invalid), or Too short: N (where \$N\$ is the length of the too-short username) on a new line for each test case.

Sample Input

3
Peter
Me
Arxwwz

Sample Output

Valid
Too short: 2
Invalid

Explanation

Username Me is too short because it only contains characters, so your exception prints Too short: 2 . All other validation is handled by the locked code in your editor.

#include <iostream>
#include <string>
#include <sstream>
#include <exception>
using namespace std;

/* Define the exception here */
struct BadLengthException : public exception {
    int number;    
    public :
        BadLengthException(int n){
            number = n;            
        }

   const char * what () const throw () {
       std::stringstream ss;
       ss << number;
       return ss.str(). c_str();

   }
};

bool checkUsername(string username) {
    bool isValid = true;
    int n = username.length();
    if(n < 5) {
        throw BadLengthException(n);
    }
    for(int i = 0; i < n-1; i++) {
        if(username[i] == 'w' && username[i+1] == 'w') {
            isValid = false;
        }
    }
    return isValid;
}

int main() {
    int T; cin >> T;
    while(T--) {
        string username;
        cin >> username;
        try {
            bool isValid = checkUsername(username);
            if(isValid) {
                cout << "Valid" << '\n';
            } else {
                cout << "Invalid" << '\n';
            }
        } catch (BadLengthException e) {
            cout << "Too short: " << e.what() << '\n';
        }
    }
    return 0;
}

Final Thoughts

• I thought it was a great idea to used a struct as opposed class as a struct is for small grouping
• Just like the previous question, I have no access to change the headers, main() and checkUsername() so I couldn't remove the using namespace std

Answer 1

This isn't really aimed at the core of your code and is more a reflection of the code challenge you're solving.

BadLengthException isn't a great name for the exception being represented. It doesn't tell the caller that the name was too short (the exception could be used to represent a length that was too long). The exception as you've implemented it has no why, so the exception itself doesn't contain sufficient information to tell the caller that it was too short / too long. With that in mind, I would have been tempted to actually implement a TooShortException and then create typedef so that the supplied code still worked:

typedef TooShortException BadLengthException;

Answer 2

• structs are public by default

  So this does not really make sense

  struct BadLengthException : public exception {
      int number;
  public :
      ...
  };
  

  Either put the declaration of number below public : or remove public :

• using namespace std

  You may be forced to keep using namespace std but your interface does not. I recommend changing

  struct BadLengthException : public exception {
  

  to

  struct BadLengthException : public std::exception {
  

• Prefer to use class here rather than a struct

  The most accepted rule I know for choosing class vs struct is whether the data members are private or not. Since you have no reason to make number public, I would make the object a class.

• Use an initializer list for the constructor

  struct BadLengthException : public std::exception {
      int number;
  public :
      BadLengthException(int n) : number(n) {}
  ...
  }
  

• Use noexcept rather than throw()

  Read here. Also note that std::exception member function what() is explicitly marked noexcept

• Use std::to_string to format the number to a string

• Use override to ensure you are overriding a virtual function in the base class

• Undefined behavior

  You are returning a pointer to a temporary in what(). If you were returning a reference in what() and the call site (code inside catch) specified storing a const reference you would be ok. While you could provide another overload to what() that masks the inherited overload, you cannot change the code in main.

  See this StackOverflow question and this GotW for related info.

  Essentially you have to store the string in the class and return a C pointer to it.

Updated Code

class BadLengthException : public std::exception {
    std::string str;  // This is private since the object is now a class

public :
    BadLengthException(int number) : str (std::to_string (number)) {}

    const char * what () const noexcept override {
        return str.c_str();
    }
};

Answer 3

maybe now is very late, but I think it is better if the username var is passed by reference. And the username should change to user_name.

bool checkUsername(const string& username) {

because pass by reference is faster, and to avoid modifying the username by an accident, should put const