Skip to content
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

Roman Androsiuk - 0 #33

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

Conversation

FlowerGunn
Copy link

Name

Roman Androsiuk

Homework#

0

Comment

Levels 1, 2 and 3

Copy link

@houndci-bot houndci-bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: We found some problems with your configuration file: [/IrresponsibleMo...
Error: We found some problems with your configuration file: [/IrresponsibleModule] key 'IrresponsibleModule:' is undefined., [/DuplicateMethodCall] key 'DuplicateMethodCall:' is undefined., [/UtilityFunction] key 'UtilityFunction:' is undefined.

same_price_find
same_price_output
end
#~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Choose a reason for hiding this comment

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

Layout/LeadingCommentSpace: Missing space after #.

@same_price_names << @sheetnow.cell(i + 8, 1)
end
end
#~~~~~~~~~~~~~~~~~~~~~Main~code~~~~~~~~~~~~~~~~~~~~

Choose a reason for hiding this comment

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

Layout/LeadingCommentSpace: Missing space after #.

(0..354).each do |i|
next unless [email protected](i + 8, 1).nil? && [email protected](i + 8, 15).nil?

next unless @sheetnow.cell(i + 8, 15) == @good_price && @sheetnow.cell(i + 8, 1) != @good_name # Item costs the same but it isn't itself

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [140/120]

puts answer # Prints answer
end

def same_price_find

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for same_price_find is too high. [18.47/15]

# Prints list of items with the same cost
answer = 'You can\'t afford anything else for that price.' # Default answer
same_length = @same_price_names.size
if same_length > 0 # Changes answer if anything was found

Choose a reason for hiding this comment

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

Style/NumericPredicate: Use same_length.positive? instead of same_length > 0.

def prices_find # Looking through all files for min and max price of the item
@allfiles = Dir.entries('./data')
allfiles_length = @allfiles.size
(0...allfiles_length).each do |i0| # Going through all files in ./data/ directory

Choose a reason for hiding this comment

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

Metrics/BlockLength: Block has too many lines. [26/25]

date # Returning a string
end

def prices_find # Looking through all files for min and max price of the item

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for prices_find is too high. [50.72/15]
Metrics/CyclomaticComplexity: Cyclomatic complexity for prices_find is too high. [11/6]
Metrics/MethodLength: Method has too many lines. [32/10]
Metrics/PerceivedComplexity: Perceived complexity for prices_find is too high. [12/7]
Style/CommentedKeyword: Do not place comments on the same line as the def keyword.

date += '2015'
elsif date_text.include? '2017'
date += '2017'
@notbyn = false # Case to convert BYR to BYN cause all files before year 2017 had prices written in 'before denomination' format

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [132/120]

end

def date_read(sheet)
# Read exact cell from requested sheet and generate mm/yyyy date

Choose a reason for hiding this comment

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

Layout/CommentIndentation: Incorrect indentation detected (column 13 instead of 2).

end
end

def date_read(sheet)

Choose a reason for hiding this comment

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

Metrics/AbcSize: Assignment Branch Condition size for date_read is too high. [59.27/15]
Metrics/CyclomaticComplexity: Cyclomatic complexity for date_read is too high. [23/6]
Metrics/MethodLength: Method has too many lines. [56/10]
Metrics/PerceivedComplexity: Perceived complexity for date_read is too high. [26/7]

@RGBD RGBD self-assigned this Dec 10, 2018
Copy link
Collaborator

@RGBD RGBD left a comment

Choose a reason for hiding this comment

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

Wrap your code into a class

require 'roo'
require 'roo-xls'
# require 'rubocop'
xlfilenow = Roo::Excelx.new('./data/Average_prices(serv)-10-2018.xlsx') # Info about prices in Minsk these days
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put code that is actually executed at the end of the file. Also you can put it all in main method and call it at the end of a file.

require 'roo-xls'
# require 'rubocop'
xlfilenow = Roo::Excelx.new('./data/Average_prices(serv)-10-2018.xlsx') # Info about prices in Minsk these days
@sheetnow = xlfilenow.sheet(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to use instance variables outside of any class

# Read exact cell from requested sheet and generate mm/yyyy date
date = '' # Actually need a tip how to make this not so dumb
date_text = sheet.cell(3, 1)
if date_text.include? 'январь'
Copy link
Collaborator

Choose a reason for hiding this comment

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

use hash to map month names to dates.

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.

3 participants