Skip to content

Pr-8 #9

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.

Sign up for GitHub

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
CheezItMan wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pr-8 #9

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1a40e1
This method flattens an array
CheezItMan Aug 15, 2019
2130149
Update flatten_array.rb
CheezItMan Aug 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions flatten_array.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
require 'pry'

class FlattenArray
def self.flatten(array)
flattened_array = []

array.each do |element|
Copy link
Copy Markdown

@AngelaOh AngelaOh Aug 15, 2019

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.

flattened_array = self.add_element_array(element, flattened_array)
end
Copy link
Copy Markdown

@jessicacaley jessicacaley Aug 15, 2019

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

flattened_array.compact
Copy link
Copy Markdown

@sophearychiv sophearychiv Aug 15, 2019

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.

end

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

@AngelaOh AngelaOh Aug 15, 2019

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.

Copy link
Copy Markdown

@jessicacaley jessicacaley Aug 15, 2019

Choose a reason for hiding this comment

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

Remove comment?

if element.class == Array
element.each do |ele|
array = self.add_element_array(ele, array)
end
else
array << element
end
array
Copy link
Copy Markdown

@jessicacaley jessicacaley Aug 15, 2019

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

Copy link
Copy Markdown

@piffer59 piffer59 Aug 15, 2019

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

end
end

module BookKeeping
Copy link
Copy Markdown

@jessicacaley jessicacaley Aug 15, 2019

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?

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

@Faiza1987 Faiza1987 Aug 15, 2019

Choose a reason for hiding this comment

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

Good job!