3
\$\begingroup\$

I have written a class to generate a unique username from the full name. The full name is the name of a company usually containing 4 to 5 words. The username generated will be of format {preffix}{username}{suffix}. The user name should not be derived from stop words. stops words are generated analysing the most common words in the list of companies name.

To generate name I am taking the first letter from each word of name and taking remaining from the last word. For eg. if the length of the username is 5 then ABC Company Limited will be converted to ACLIM.

kindly review the code.

class UniqueNameGenerator:

    def __init__(self, stop_words, suffix, prefix):

        self.stop_words = stop_words
        self.suffix = suffix
        self.prefix = prefix

    def split_full_name_and_filter_stop_word(self, full_name, size):

        name_words = " ".join(full_name.split(" ")).split()

        if len(name_words) > size:
            name_words = name_words[:size]
        words_considered = []
        for word in name_words:
            ignore_word = False
            if word.lower() in self.stop_words:
                ignore_word = True
            if not ignore_word:
                words_considered.append(word)
        return words_considered

    @staticmethod
    def get_name_from_list_of_words(size, list_of_words):

        size_counter = 1
        len_words_considered = len(list_of_words)
        letters_left = size
        username = ""
        while size_counter <= len_words_considered:
            current_word = list_of_words[size_counter - 1]
            if size_counter == len_words_considered:
                if len(current_word) >= letters_left:
                    username += current_word[:letters_left]
                else:
                    username += current_word
            else:
                username += current_word[0]
                letters_left -= 1
            size_counter += 1
        return username

    def check_and_update_username_to_unique(self, username, unique_name_list):

        while True:
            s = len(username)
            if username in unique_name_list:
                char_list = list(username)
                char_list[random.randint(0, s)] = random.choices(
                    string.ascii_uppercase + string.digits
                )[0]
                username = "".join(char_list)
            else:
                return username

    def generate_username(self, full_name, size, unique_name_list=None):

        if not unique_name_list:
            unique_name_list = []

        full_name = re.sub("[^\sa-zA-Z]+", "", full_name)
        words_considered = self.split_full_name_and_filter_stop_word(full_name, size)
        username = self.get_name_from_list_of_words(size, words_considered)
        if self.suffix:
            username = username + self.suffix
        if self.prefix:
            username = self.prefix + username
        username = self.check_and_update_username_to_unique(username, unique_name_list)
        return username
\$\endgroup\$
  • \$\begingroup\$ Can you expand on what you mean by stop words? Some clarification on the whole full name vs username vs name would be good, too. For example, the sentence The username generated will be of format {preffix}{username}{suffix}. seems to indicate some sort of recursion in the username. \$\endgroup\$ – Alexander Cécile Nov 14 at 21:06
  • \$\begingroup\$ what should be the result for full_name= 'ABC Company Limited' and size=2 ? \$\endgroup\$ – RomanPerekhrest Nov 14 at 21:10
  • \$\begingroup\$ @RomanPerekhrest Good question. In the same vein, what would the username be for full_name= 'ABC Company Limited' and size=10+? \$\endgroup\$ – Alexander Cécile Nov 14 at 21:12
  • \$\begingroup\$ @RomanPerekhrest username should be 'ac'. \$\endgroup\$ – Joshi R Nov 15 at 2:13
  • \$\begingroup\$ @AlexanderCécile currently it would print only 'abclimited'. I missed this case. \$\endgroup\$ – Joshi R Nov 15 at 2:18
3
\$\begingroup\$

Areas to fix/improve:

Namings

The function names like split_full_name_and_filter_stop_word, check_and_update_username_to_unique and alike are considered as anti-patterns for function naming.
run_and_fly or think_or_talk like names point to ambiguity or excessive responsibilities assigned to a function.
A function should have a concrete responsibility. Therefore, you need to analyze those functions and apply whether Rename function or Extract function (in your case - split a function into several functions each with separate responsibility) technique.

Some variable and function names are too verbose:
name_words --> just names
get_name_from_list_of_words --> just compose_username
list_of_words --> just a plural words
unique_name_list --> just unique_names


def generate_username(self, full_name, size, unique_name_list=None) method.

  • to avoid passing the same unique_name_list across multiple methods - it's good to pass it into UniqueNameGenerator constructor at once.

  • full_name = re.sub("[^\sa-zA-Z]+", "", full_name). In case if full_name happened to be empty, to avoid redundant calls of subsequent functions - it's good to add a check for that:

    ...
    full_name = re.sub("[^\sa-z]+", "", full_name.strip(), re.I)
    if not full_name:
        raise ValueError(f'Incorrect full name `{full_name}`')
    
  • either of these complements:

    if self.suffix:
        username = username + self.suffix
    if self.prefix:
        username = self.prefix + username
    

    can throw TypeError in case if caller would pass non-string argument for prefix or suffix.
    Instead, use flexible f-string formatting:

    username = f'{self.prefix}{username}{self.suffix}'
    

def split_full_name_and_filter_stop_word(self, full_name, size) method

  • the function name is better named as filter_valid_words or select_valid_words (or alike)
  • " ".join(full_name.split(" ")).split() - this looks like a "5-wheel bicycle that circles rounds instead of going straight".
    Simply words = full_name.split()

  • the whole construction:

    words_considered = []
    for word in name_words:
        ignore_word = False
        if word.lower() in self.stop_words:
            ignore_word = True
        if not ignore_word:
            words_considered.append(word)
    

    is a verbose "invention" of a simple list comprehension with if constraint:

    valid_words = [w for w in words if w.lower() not in self.stop_words]
    

    The restructured function would look as:

    def filter_valid_words(self, full_name, size):
        words = full_name.split()
        if len(words) > size:
            words = words[:size]
    
        return [w for w in words if w.lower() not in self.stop_words]
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.