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

Recreating Ruby's Enumerable #3

Merged
merged 23 commits into from Feb 25, 2020
Merged

Recreating Ruby's Enumerable #3

merged 23 commits into from Feb 25, 2020

Conversation

EdCrux
Copy link
Owner

@EdCrux EdCrux commented Feb 20, 2020

Thanks in advance for the review. 🚀
This is the development summarize path of recreating Ruby's Enumerable.
my_each, my_each_with_index, my_select, my_all?, my_any? , my_none? my_counter? my_map? and my_inject?

  • Analyzing the original method does.
  • Clarifying the algorithm
  • Testing with different possibilities and values
  • Building the method
  • Debugging and comparing with the original

Adding enumerables file
Adding Rubocop, stickler, and the main strutucture of the readme.md file.
Adding my_each and my_each_with_index method that helps to pass through an array or hash to get the values
This method is a filter for array and hashes, it is a copy of select method inside ruby
This method evaluates if each element of the array meets the condition and returns true if all elements meet the condition.
This method evaluates if your are passing a block or a pattern, then iterates into each element and returns true if none element match with the condition
This method provides helps to inject new code to an existing array or range
Fixing bugs my_all? and my_none? methods
Checking bugs when you pass nil as value in the array
Removing linter warnigns with rubocop, debugging and testing myall? adding the logic for working without blocks
Updating link for live preview
@Anna-Myzukina
Copy link

Pull Request under the review by Anna Muzykina (TSE)

@Anna-Myzukina
Copy link

Hi @EdCrux !

Status: [changes required]

  • Good job so far, just a couple more changes and your project can be approved!

Next methods still working not correct:

  • my_all? this method should return the same thing as ruby's all?

  • my_any? this method should return the same thing as ruby's any?

  • my_none? this method should return the same thing as ruby's none?

  • my_inject this method should return the same thing as ruby's inject

Make changes and submit another code review request.

If you have any questions, let me know on slack (@Anna-Muzykina) and I will be glad to clarify with you.
*Happy coding *

Fixing bugs, and making testing to catch any error
Catching if there are no error in some of the methods
Copy link

@fatymahmed fatymahmed left a comment

Choose a reason for hiding this comment

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

Hi @EdCrux,

Great job so far 👏👏, most of your methods are working properly, just a few changes and you are good to go.

  • Check my comments below for a detailed review of your methods. You can also use this as a guide to see the enumerable methods and the different ways each are used (some have different arguments passed).

[OPTIONAL]

  • Please don't change stickler config as mentioned on Pathwright, see screenshot, the only exception is

You can have Style/CaseEquality cop disabled as described here see Screenshot from Pathwright

Marking this as optional because it was missed in the first review, however, you are highly encouraged to make this change.

Status: Changes Required 🛑
Feel free to reach out to me on slack (Fatima) if you have any questions, I'd be happy to help

enumerables.rb Outdated
Comment on lines 54 to 70
def my_all?(data = nil)
return my_none?(data) if block_given? && !data.nil?

arr = to_a
return true if arr.empty?

if block_given?
to_a.my_each { |item| return false unless yield(item) }
elsif data.nil? && !block_given?
to_a.my_each { |item| return false unless item }
elsif data.is_a? Class
to_a.my_each { |item| return false unless item.is_a? data }
elsif data.is_a? Regexp
to_a.my_each { |item| return false unless item.to_s.match(data) }
end
true
end

Choose a reason for hiding this comment

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

  • For my_all? handle the case when a pattern other than Regex or a Class is given, it should return true if all of the collection matches the pattern.

For example
The built-in method
[2,3,4,5,6].all?(3) returns false

Your method
[2,3,4,5,6].my_all?(3) returns true

  • It should return false since not all elements in the array are 3

enumerables.rb Outdated
Comment on lines 72 to 86
def my_any?(data = nil)
return my_any?(data) if block_given? && !data.nil?

if block_given?
to_a.my_each { |item| return true if yield item }
false
elsif data.is_a? Regexp
to_a.my_each { |item| return true if item.to_s.match(data) }
elsif data.is_a? Class
to_a.my_each { |item| return true if item.is_a? data }
else
to_a.my_each { |item| return true if item == data }
end
false
end
Copy link

Choose a reason for hiding this comment

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

  • For my_any? handle the case when no block or argument is given returns true if at least one of the collection is not false or nil

For example:
The built-in method
[nil, false, nil, false].any? returns false

Your method
[nil, false, nil, false].my_any? returns true

  • It should return false since all of the elements are either false or nil.

enumerables.rb Outdated
Comment on lines 88 to 101
def my_none?(data = nil)
return my_none?(data) if block_given? && !data.nil?

if block_given?
to_a.my_each { |item| return false if yield(item) }
elsif data.is_a? Regexp
to_a.my_each { |item| return false if item.to_s.match(data) }
elsif data.is_a? Class
to_a.my_each { |item| return false if item.is_a? data }
elsif data.nil?
to_a.my_each { |item| return false if item }
end
true
end

Choose a reason for hiding this comment

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

  • For my_none? Handle the case when a pattern other than Regex or a Class is given, it returns true only if none of the collection matches the pattern

For example
Built-in method
[5, 'door', 'rod', 'blade'].none?(5) returns false

Your method
[5, 'door', 'rod', 'blade'].my_none?(5) returns true

  • It should return false since at least 1 element matches the pattern.

enumerables.rb Outdated
Comment on lines 126 to 140
def my_inject(*initial)
arr = to_a
return raise ArgumentError, 'Given arguments 0, expected at least 1' if initial.empty? && !block_given?

memo = initial.length == 2 && arr.respond_to?(initial[1]) || initial.length == 1 && block_given? ? initial[0] : arr.shift
sym = if initial.length == 2
initial[1]
elsif !block_given? && initial.length == 1 && arr.respond_to?(initial[0])
initial[0]
else
false
end
arr.my_each { |item| memo = sym ? memo.send(sym, item) : yield(memo, item) }
memo
end

Choose a reason for hiding this comment

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

  • For my_inject handle the cases
  1. when a symbol is specified, it combines each element of the collection by applying the symbol as a named method
  • Eg array.my_inject(:+)
  1. it combines all elements of enum by applying a binary operation, specified by a block
  • Eg Range.new(5, 50).{ |sum, n| sum + n }

Adding some logic to give my_none, my_any and my_all? acept parameter such as numbers
Removing files such as spec folder and rspec
Addign logic to the method to recieve numbers or strings as an argument and start comparing
Removing linter warnings
@shshamim63
Copy link

Status ♠️ [Required Changes] ♠️


Hi @EdCrux 👋 ,
Awesome job :clap. Code looks neat and clean and you have covered all the changes asked by the previous reviewer. But I have found three cases where some of the methods malfunctions. The good news is by the pace you are covering the issue I am sure you will nail those too. So lets get to those issues.


👾 [Mandatory Changes] 👾


  1. my_any? identical and returns the same thing as ruby's any?. (Screenshot from Odin)
    def my_any?(data = nil)
    return my_any?(data) if block_given? && !data.nil?
    if block_given?
    to_a.my_each { |item| return true if yield item }
    false
    elsif data.is_a? Regexp
    to_a.my_each { |item| return true if item.to_s.match(data) }
    elsif data.is_a? Class
    to_a.my_each { |item| return true if item.is_a? data }
    elsif data
    to_a.my_each { |item| return true if item == data }
    end
    false
    end

    ...1. enumerables my_any? when no block or argument is given returns true if at least one of the collection is not false or nil.
puts [1, nil, false].my_any?   # should return true.
puts [nil, nil, false].my_any?   # should return false. Which I am getting on your one.
  1. my_inject identical and returns the same thing as ruby's inject. (Screenshot from Odin)
    def my_inject(*initial)
    arr = to_a
    return raise ArgumentError, 'Given arguments 0, expected 1' if initial.empty? && !block_given?
    memo = initial.length == 2 && arr.respond_to?(initial[1]) || initial.length == 1 && block_given? ? initial[0] : arr.shift
    sym = if initial.length == 2
    initial[1]
    elsif !block_given? && initial.length == 1 && arr.respond_to?(initial[0])
    initial[0]
    else
    false
    end
    arr.my_each { |item| memo = sym ? memo.send(sym, item) : yield(memo, item) }
    memo
    end

    ...1. my_inject, when a symbol is specified combines each element of the collection by applying the symbol as a named method. But for some cases I am not getting the desired ans that is required. For an example lets execute the following code, after the end of your enumerable module
ARRAY_SIZE = 5
LOWEST_VALUE = 0
HIGHEST_VALUE = 9
array = Array.new(ARRAY_SIZE) { rand(LOWEST_VALUE...HIGHEST_VALUE) }
puts array.my_inject(:+)
puts array.inject(:+)

should get same result printed twice. But not rendering it.


♣️ [Optional Changes] ♣️

  1. multiply_els that uses my_inject as described on Odin (Screenshot from Odin)
    There must be a method called multiply_els which uses my_inject. The main purpose of that method is to check your enumerable methods are accessible from outside and you know how to use them properly. But I can not see that on this project. And I am sorry that the previous reviewer did not mention about that one too. So marking this one as optional.

Please fix the above mentioned issue and send another review request.


🌿 Happy Coding 🌿

Shakhawat Hossain

Linkedin

Adding a condition that returns true or false when youe don't pass an argument through the array
Copy link

@Forison Forison left a comment

Choose a reason for hiding this comment

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

Status Approved💚 💚 💚

Good job on addressing all the issues addressed in the previous reviews🙂

** [Optional] **

as mentioned in the previous review

There must be a method called multiply_els which uses my_inject. The main purpose of that method is to check your enumerable methods are accessible from ** outside **(outside the module) and you know how to use them properly" multiply_els has to be moved outside the module.

That is it, proceed to the next task

Happy coding

Addo Forison

@EdCrux EdCrux merged commit d1de004 into development Feb 25, 2020
@EdCrux EdCrux deleted the add-enumerable branch February 25, 2020 20:35
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.

None yet

5 participants