Skip to content

Pr-8#9

Open
CheezItMan wants to merge 2 commits intomasterfrom
pr-8
Open

Pr-8#9
CheezItMan wants to merge 2 commits intomasterfrom
pr-8

Conversation

@CheezItMan
Copy link
Copy Markdown

No description provided.

Comment thread flatten_array.rb

array.each do |element|
flattened_array = self.add_element_array(element, flattened_array)
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You might want to add a new line here.

Suggested change
end
end

Comment thread flatten_array.rb
end

def self.add_element_array(element, array)
# binding.pry
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove comment?

Comment thread flatten_array.rb
else
array << element
end
array
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

An explicit return might make this more legible.

Suggested change
array
return array

Comment thread flatten_array.rb
end
end

module BookKeeping
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It might just be me, but it's a little hard to understand what this is doing. Could you maybe add a comment for context?

Comment thread flatten_array.rb
end

def self.add_element_array(element, array)
# binding.pry
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# binding.pry

Consider removing this line if we are wanting to push to prod.

Comment thread flatten_array.rb
def self.flatten(array)
flattened_array = []

array.each do |element|
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using a .map instead of .each. This would not require declaring var in line 5.

Comment thread flatten_array.rb

module BookKeeping
VERSION = 1 # Where the version number matches the one in the test.
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job!

Comment thread flatten_array.rb
else
array << element
end
array
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

including return would be helpful

Comment thread flatten_array.rb
array.each do |element|
flattened_array = self.add_element_array(element, flattened_array)
end
flattened_array.compact
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

compact returns a copy of the array which increases space complexity.

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.

6 participants