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

add a new functions - Remove #172

Closed
wants to merge 6 commits into from
Closed

add a new functions - Remove #172

wants to merge 6 commits into from

Conversation

Luoxin
Copy link
Contributor

@Luoxin Luoxin commented Sep 6, 2021

#171


This change is Reviewable

@Luoxin
Copy link
Contributor Author

Luoxin commented Sep 6, 2021

#171

@@ -81,6 +81,7 @@ var Functions = []struct {
{"Unique", "unique.go", ForNumbersAndStrings, "n"},
{"Unshift", "unshift.go", ForAll, "n"},
{"Values", "values.go", ForMaps, "n"},
{"Remove", "remove.go", ForNumbersAndStrings, "n⋅n"},
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep these lines sorted by name.

Also, O(n^2) isn't quite correct, it would be O(kn) but more on that below.

functions/remove.go Show resolved Hide resolved
functions/remove.go Show resolved Hide resolved
@@ -0,0 +1,19 @@
package functions

// Remove items from slice when item existed
Copy link
Owner

Choose a reason for hiding this comment

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

// Remove returns a new slice that does not include any of the items.

@elliotchance
Copy link
Owner

@Luoxin thanks for creating this diff, with a few tweaks I'm happy to merge. You will also need to update the README.

- change n^2 to 2n
@Luoxin
Copy link
Contributor Author

Luoxin commented Sep 7, 2021

Thank you for the correction, I made some changes

README.md Outdated
| [`Unique`](#unique) | ✓ | ✓ | | | n | Unique returns a new slice with all of the unique values. |
| [`Unshift`](#unshift) | ✓ | ✓ | ✓ | | n | Unshift adds one or more elements to the beginning of the slice and returns the new slice. |
| [`Values`](#values) | | | | ✓ | n | Values returns the values in the map. |
| [`SortUsing`](#sortusing) | ✓ | | ✓ | | n⋅log(n) | SortUsing works similar to sort.Slice. However,
Copy link
Owner

Choose a reason for hiding this comment

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

The format of the table has been mangled. Please fix.

README.md Outdated
Due to Go's randomization of iterating maps the order is not deterministic.

## Remove
Copy link
Owner

Choose a reason for hiding this comment

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

Keep the titles sorted.

@@ -81,7 +81,7 @@ var Functions = []struct {
{"Unique", "unique.go", ForNumbersAndStrings, "n"},
{"Unshift", "unshift.go", ForAll, "n"},
{"Values", "values.go", ForMaps, "n"},
{"Remove", "remove.go", ForNumbersAndStrings, "n⋅n"},
{"Remove", "remove.go", ForNumbersAndStrings, "2n"},
Copy link
Owner

@elliotchance elliotchance Sep 7, 2021

Choose a reason for hiding this comment

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

O(2n) is not a valid complexity, the constant would be removed to O(n). Also, you must keep the functions sorted.

- readme
README.md Outdated
the slice and returns the new slice. | | [`Values`](#values) | | | | ✓ | n | Values returns the
values in the map. | | [`Remove`](#values) | ✓ | ✓ | | ✓ | 2n | Remove returns a new slice that does
not include any of the items. |
| [`SortUsing`](#sortusing) | ✓ | | ✓ | | n⋅log(n) | SortUsing works similar to sort.Slice. However, unlike sort.Slice the slice returned will be reallocated as to not modify the input slice. |
Copy link
Owner

Choose a reason for hiding this comment

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

This is still mangled, you can repair this by restoring the original file:

git checkout origin/master -- README.md

Now add the Remove line back in and commit again.

@@ -216,6 +216,7 @@ The letters in brackets indicate:
| [`Product`](#product) | | ✓ | | | n | Product is the product of all of the elements. |
| [`Random`](#random) | ✓ | ✓ | ✓ | | 1 | Random returns a random element by your rand.Source, or zero |
| [`Reduce`](#reduce) | ✓ | ✓ | | | n | Reduce continually applies the provided function over the slice. Reducing the elements to a single value. |
| [`Remove`](#remove) | ✓ | ✓ | | ✓ | n | Remove returns a new slice that does not include any of the items. |
Copy link
Owner

Choose a reason for hiding this comment

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

Maps should not be ticked.

Did you format this file after running generate.go?

@elliotchance
Copy link
Owner

@Luoxin - if you want to revisit this, you can build against v2 which is much more flexible.

@Luoxin
Copy link
Contributor Author

Luoxin commented Mar 24, 2022

I will do something in this weekend

This pull request was closed.
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.

2 participants