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

PR-8 #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

PR-8 #8

wants to merge 1 commit into from

Conversation

CheezItMan
Copy link

This method flattens an array

@tildeee tildeee changed the title Create flatten_array.rb PR-8 Feb 6, 2020
end

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

Choose a reason for hiding this comment

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

You might want to remove this line before publication.

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

@llinneaa llinneaa Feb 6, 2020

Choose a reason for hiding this comment

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

Replace .compact with .compact! to modify the original array.

Choose a reason for hiding this comment

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

If we added a ! it would return nil if no changes were made, which we might want to avoid

# binding.pry
if element.class == Array
element.each do |ele|
array = self.add_element_array(ele, array)

Choose a reason for hiding this comment

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

nice use of recursion

else
array << element
end
array

Choose a reason for hiding this comment

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

return array? or do something with array?


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

Choose a reason for hiding this comment

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

Add a line at the end!

end

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

Choose a reason for hiding this comment

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

please remove the comment after debugging!

flattened_array.compact
end

def self.add_element_array(element, array)

Choose a reason for hiding this comment

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

please change the variables to be more clear to the code( element and array)

@@ -0,0 +1,28 @@
require 'pry'

Copy link

Choose a reason for hiding this comment

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

remove the debugging stuff, or comment it out?

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

Choose a reason for hiding this comment

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

What is the runtime of @compact?

# binding.pry
if element.class == Array
element.each do |ele|
array = self.add_element_array(ele, array)
Copy link

Choose a reason for hiding this comment

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

Is this recursion necessary? Clever, but I feel that we may be sacrificing too much space.

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

Choose a reason for hiding this comment

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

The .compact method is making a copy here, this method could use a ! (compact!) or be saved to a new variable name with the non-mutating version.

else
array << element
end
array
Copy link

Choose a reason for hiding this comment

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

Recommended: make explicit return

end

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

Choose a reason for hiding this comment

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

remove binding.pry

flattened_array.compact
end

def self.add_element_array(element, array)
Copy link

Choose a reason for hiding this comment

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

rename method (unclear functionality based on name)

def self.add_element_array(element, array)
# binding.pry
if element.class == Array
element.each do |ele|
Copy link

Choose a reason for hiding this comment

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

rename 'ele'

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.

9 participants