Handle possible duplicates in random tags generated by tag_torrent #6820
Conversation
…sible duplicates in random tags
kozlovsky
requested review from
a team and
xoriole
and removed request for
a team
|
Kudos, SonarCloud Quality Gate passed!
|
|
retest this please |
| if tags is None: | ||
| tags_count = random.randint(2, 6) | ||
| tags = [] | ||
| while len(tags) < tags_count: | ||
| tag = get_random_word(min_length=MIN_TAG_LENGTH) | ||
| if tag not in tags: | ||
| tags.append(tag) | ||
|
|
||
| if suggested_tags is None: | ||
| suggested_tags_count = random.randint(1, 3) | ||
| suggested_tags = [] | ||
| while len(suggested_tags) < suggested_tags_count: | ||
| tag = get_random_word(min_length=MIN_TAG_LENGTH) | ||
| if tag not in suggested_tags: | ||
| suggested_tags.append(tag) |
There was a problem hiding this comment.
NIT, noncritical: you use the same pattern twice. It is possible to extract this pattern:
def generate_sentence_with_fixed_word_length(words_count: int, min_word_length) -> List[str]:
sentence = []
while len(sentence) < words_count:
word = get_random_word(min_length=min_word_length)
if word not in sentence:
sentence.append(word)
return sentenceand refactor the code block to:
| if tags is None: | |
| tags_count = random.randint(2, 6) | |
| tags = [] | |
| while len(tags) < tags_count: | |
| tag = get_random_word(min_length=MIN_TAG_LENGTH) | |
| if tag not in tags: | |
| tags.append(tag) | |
| if suggested_tags is None: | |
| suggested_tags_count = random.randint(1, 3) | |
| suggested_tags = [] | |
| while len(suggested_tags) < suggested_tags_count: | |
| tag = get_random_word(min_length=MIN_TAG_LENGTH) | |
| if tag not in suggested_tags: | |
| suggested_tags.append(tag) | |
| if tags is None: | |
| tags_count = random.randint(2, 6) | |
| tags = generate_sentence_with_fixed_word_length(tags_count, MIN_TAG_LENGTH) | |
| if suggested_tags is None: | |
| suggested_tags_count = random.randint(1, 3) | |
| suggested_tags = generate_sentence_with_fixed_word_length(suggested_tags_count, MIN_TAG_LENGTH) |
There was a problem hiding this comment.
Thank you for the suggestion, but I'm not persuaded we should do it, for the following reasons:
-
A usual "rule of thumb" is "if the same code is repeated three or more times, consider to extract it into a function". Here the same pattern is repeated only twice.
-
The version with function does not make the code shorter.
-
It is not obvious that the function will be reusable in other places. In other situations it may be totally ok to have repeated words in a sentence.
-
Right now all repeated patterns are inside the same function, extracting the code to a second function makes the code "less local" and so a bit more complex to analyze (as the reader cannot assume that the new function is not used from other places).
There was a problem hiding this comment.
Agree, but the rationale behind referenced "rule of thumb" is the following:
Attempting premature refactoring risks selecting a wrong abstraction, which can result in worse code as new requirements emerge and will eventually need to be refactored again.
In the code above suggested abstraction is clear and correct. It extracts exactly the same logic to the function with a very clear intention.
The version with function does not make the code shorter.
Agree, but shorter doesn't mean better.
The suggested refactoring code is easier to read because it doesn't require from reader to analyze the code blocs and to determine the pattern.
It is not obvious that the function will be reusable in other places. In other situations it may be totally ok to have repeated words in a sentence.
Agree, you can make it local.
Right now all repeated patterns are inside the same function, extracting the code to a second function makes the code "less local" and so a bit more complex to analyze (as the reader cannot assume that the new function is not used from other places).
Agree, you can make it local.
Fixes #6819