Skip to content

Conversation

@Slavunderkind
Copy link
Owner

No description provided.

@Slavunderkind Slavunderkind requested review from a user and vasil-gochev January 11, 2017 16:04
@Slavunderkind Slavunderkind changed the title Update mime type solution [Mime type task] Update mime type solution Jan 12, 2017
mime_type.rb Outdated
end

def match_file(filename)
splitted_fname = filename.split('.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not mix abbreviated variable names with full variable names. In Ruby, it's best to always use the full variable names.

mime_type.rb Outdated
splitted_fname = filename.split('.')
result = 'UNKNOWN'
if splitted_fname.size > 1 && filename[-1] != '.'
result = associations.fetch(splitted_fname.last.downcase, 'UNKNOWN')
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can simplify the logic here:
You start with a result of 'UNKNOWN'. You check some condition and if it's true, you use Hash#fetch to either return a result, or provide the same default value of 'UNKNOWN'.

Try refactoring it in a way that would use 'UNKNOWN' only once

mime_type.rb Outdated

def start
q.times do
fname = gets.chomp # One file name per line.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use full variable names

@Slavunderkind Slavunderkind force-pushed the master branch 4 times, most recently from b8e5d88 to 73f6435 Compare January 17, 2017 14:56
def match_file(filename)
unknown = 'UNKNOWN'
splitted_filename = filename.split('.')
if splitted_filename.size > 1 && filename[-1] != '.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's currently going to happen if that condition evaluates to false?

Copy link
Owner Author

Choose a reason for hiding this comment

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

@vasil-gochev result is 'UNKNOWN'

unknown = 'UNKNOWN'
splitted_filename = filename.split('.')
if splitted_filename.size > 1 && filename[-1] != '.'
result = associations.fetch(splitted_filename.last.downcase, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need Hash#fetch here at all - you can use Hash#[] instead

if splitted_filename.size > 1 && filename[-1] != '.'
result = associations.fetch(splitted_filename.last.downcase, nil)
end
result = unknown if result.nil?
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to check if the result is explicitly nil

end

def start
q.times do
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does q mean?

Copy link
Owner Author

Choose a reason for hiding this comment

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

See line 7

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know where to find it - I meant its name can be much more descriptive.

A clear indication of that is that you have to write comments to describe what your variables represent. Instead, you can just name them better and the comments won't be necessary

Copy link
Owner Author

Choose a reason for hiding this comment

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

I understood. This was left from the example code added on the task. I will fix it.


def match_file(filename)
unknown = 'UNKNOWN'
splitted_filename = filename.split('.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you get when you split a filename?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants