3
\$\begingroup\$

I made program to open a text file and count the number of lines without doubled words. This number is printed in the end.

This is an example text file:

sayndz zfxlkl attjtww cti sokkmty brx fhh suelqbp
xmuf znkhaes pggrlp zia znkhaes znkhaes
nti rxr bogebb zdwrin
sryookh unrudn zrkz jxhrdo zrkz
bssqn wbmdc rigc zketu ketichh enkixg bmdwc stnsdf jnz mqovwg ixgken

I've already made a program, and it looks like that program works. But I'm aware that in programming if something works it doesn't mean that program is made properly.

My code:

class SkyphrasesValidation(object):
    def get_text_file(self):
        file = open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r')
        return file

    def lines_list(self):
        text = self.get_text_file()
        line_list = text.readlines()
        return [line.split() for line in line_list]

    def phrases_validation(self):
        validated_phrases = 0
        for line in self.lines_list():
            new_line = []
            for word in line:
                exam = line.count(word)
                if exam > 1:
                    new_line.append(0)
                else:
                    new_line.append(1)
            if 0 in new_line:
                validated_phrases += 0
            else:
                validated_phrases += 1
        return validated_phrases

    def __str__(self):
        return str(self.phrases_validation())

text = SkyphrasesValidation()

print(text)

Is my logic good and is this program is well-made? Or maybe it looks like poop and I could write this more clearly?

\$\endgroup\$
2
  • \$\begingroup\$ When you say "doubled words" do you mean the same word immediately repeated, like "the the" or do you mean a line that contains the same word twice, possibly with words in between, like "if I could I would"? \$\endgroup\$
    – aghast
    Apr 19 '19 at 19:47
  • \$\begingroup\$ I mean that in one line are 2 or even more same words. \$\endgroup\$
    – Kuracha
    Apr 19 '19 at 22:58
3
\$\begingroup\$

Here are some suggestions:

Keep file handlers in one function.

First off, the get_text_file() and lines_list() methods:

def get_text_file(self):
    file = open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r')
    return file

def lines_list(self):
    text = self.get_text_file()
    line_list = text.readlines()
    return [line.split() for line in line_list]
  1. It's generally a bad idea to pass files outside of a function... it poses some questions: Who will close the file? When? And how? Instead, open and close it in the same function so that the file can be contained.

  2. Since the lines_list() method is only reading text off from get_text_file() (and nothing else), it may be better to simply write one function which reads handles and reads lines off the file. (The splitting of lines, line.split(), can be done in phrases_validation().)

With these points in mind, we could define get_text_lines() as:

def get_text_lines(self):
    with open('C:/Users/PC/Documents/skychallenge_skyphrase_input.txt', 'r') as file:
        line_list = file.readlines()

    return line_list

Notice that the use of the with statement. This automatically closes the file when the block-scope is exited.

Using the above function implies modifying phrases_validation():

def phrases_validation(self):
    # ...
    for line in get_text_lines():   # call function here
        # ...
        words = line.split()        # split lines here
        for word in words:          # iterate through split line here
            # ...

The Validation Algorithm

I've tried annotating the meat of your phrases_validation:

# iterate through lines...
for line in get_text_lines():

    new_line = []  # container to store 1s and 0s

    # iterate through words...
    words = line.split()
    for word in words:

        exam = line.count(word)

        # append 0 if word is duplicated
        if exam > 1:
            new_line.append(0)

        # append 1 if word is not duplicated
        else:
            new_line.append(1)

    # increment validated_phrases if no duplicates
    if 0 in new_line:
        validated_phrases += 0
    else:
        validated_phrases += 1

Some general suggestions:

  1. Opt for readability. Since only 0 and 1 are being used, this implies a binary domain, and thus can be substituted with False and True respectively.
  2. validated_phrases += 0 This can be removed since nothing is added. Simply do

    if 0 not in new_line:
        validated_phrases += 1
    

Some possible improvements to the algorithm:

Using the any built-in function.

The any built-in function takes a list and tests for truth. If any of the elements are Truthy, any returns True, else False. Your inner loop can make use of the any function:

for line in get_text_lines():
    words = line.split()

    has_duplicates = any(words.count(word) > 1 for word in words)
    if not has_duplicates:
        validated_phrases += 1

Here, a comprehension was used: words.count(word) > 1 for word in words.

This generates an iterable of True/False values: True if words.count(word) > 1 and False otherwise. We then check whether there are any duplicates and if not, increment validated_phrases.

(To keep track of the number of duplicates, you can use the sum function instead. This will add the True values, counting the number of duplicates.)

Using the set class.

Indeed, there are a couple more ways to check for duplicates. list.count has a time-complexity with an upper-bound of \$O(n)\$ (with \$n\$ being the length of the list), i.e. it searches the entire list, which may be slow when the list grows to sizes of thousands or millions.

One other way to check for duplicates would be to obtain a container of unique words, and check whether it has the same length as the list of words in the line. We can use set to obtain the unique words.

for line in get_text_lines():
    words = line.split()

    unique_words = set(words)
    has_duplicates = len(unique_words) != len(words)
    if not has_duplicates:
        validated_phrases += 1,

I'm not entirely sure about the complexity of set(words) but the len function has a time complexity of \$O(1)\$ which can be really fast in the long run.

Using the collections.Counter class.

Another way that might be of interest would be to use Counter from the collections module.

for line in get_text_lines():
    words = line.split()

    counter = Counter(words)
    has_duplicates = any(counter[word] != 1 for word in counter)
    if not has_duplicates:
        validated_phrases += 1

(For this, remember to add from collections import Counter at the top of your script.)

Try not to rely on __str__.

In your code, the main algorithm, phrases_validation(), is only called when str(text) or text__str__() is called (this is implicitly called by print(text)).

It's creative... but prefer to directly call phrases_validation():

validator = SkyphrasesValidation()
print(validator.phrases_validation())

(And call str() on it if needed.) This is more explicit to readers that you're calling the phrases_validation() method.

Generally, the __str__ magic method should be overloaded when you're trying to display a class' members contents. Unfortunately, SkyphrasesValidation doesn't have any members.

The variable name text can be interpreted in several ways. Is it a text file? A string? Consider a more descriptive name instead. (validator was used in the example above.)

Class?

The class could have been substituted with one/two functions. But sticking with a class... since the self argument is rarely used, consider making the methods static or instead, place get_text_lines() outside of the class and pass the lines of text to phrases_validation() as an argument.

Hope this helps!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ There are a lot of good suggestions in this answer, unfortunately bracketed by some ungood language. You might want to read the Code of Conduct again, and be a bit more friendly. \$\endgroup\$
    – aghast
    Apr 19 '19 at 23:34
  • \$\begingroup\$ Thanks, it was very helpful, I closed all program in one function \$\endgroup\$
    – Kuracha
    Apr 20 '19 at 0:34
  • \$\begingroup\$ @AustinHastings Hmm, okay. I've tried to tidy the answer some more. I'll try to catch hold of my fingers before typing next time. Thanks for the tip! \$\endgroup\$
    – TrebledJ
    Apr 20 '19 at 5:29

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.