Skip to content

Python-Home-Challenges-Maor Weiss #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Python-Home-Challenges-Maor Weiss #10

wants to merge 4 commits into from

Conversation

MaorW
Copy link

@MaorW MaorW commented Nov 29, 2019

biggest_counting = 0
count = 0

copy_list = split_words_from_file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason to declare new variable here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean new variables? or just count variable..? @AviadP

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no reason just to assign "split_words_from_file" into "copy_list"

else:
count = 0

print 'The word "{}" which has appeared {} times is the most recurring word in the file.'.format(the_most_recurring_word, biggest_counting)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syntax error, missing '('
also, using f' string is considered as best practice for python 3

count = 0

print 'The word "{}" which has appeared {} times is the most recurring word in the file.'.format(the_most_recurring_word, biggest_counting)
fp.close()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using close is important, but in case of error or exception, your script wont get to that point, and file will remain open. best practice is is to use "with...open..." for files operations

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on it. But why my script won't get to that point?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in case of exception in one of previous lines, like syntax error, type error etc.

Copy link
Author

@MaorW MaorW left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code enhancement

@MaorW
Copy link
Author

MaorW commented Dec 1, 2019

@AviadP
Do you see my new commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants