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

List interface lacks simple remove(index) method #947

Closed
sethladd opened this issue Dec 22, 2011 · 19 comments
Closed

List interface lacks simple remove(index) method #947

sethladd opened this issue Dec 22, 2011 · 19 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug

Comments

@sethladd
Copy link
Contributor

Let's add a remove(index) method to List. Something like http://docs.oracle.com/javase/1.4.2/docs/api/java/util/List.html#remove(int)

@floitschG
Copy link
Contributor

Removed Type-Defect label.
Added Type-Enhancement, Area-Library, Triaged labels.

@sethladd
Copy link
Contributor Author

A proposal is to name the method removeAt(index) to be more clear this is "remove the object at this index" and not "find the object and then remove it

@sethladd
Copy link
Contributor Author

sethladd commented Aug 3, 2012

cc @lrhn.
Added Library-Core label.

@lrhn
Copy link
Member

lrhn commented Aug 4, 2012

Added Accepted label.

@jmesserly
Copy link

+1, had to create my own "utils" with a "removeAt" method yet again ...

@sethladd
Copy link
Contributor Author

Added this to the Later milestone.

@jmesserly
Copy link

I hope Milestone-Later means Milestone-Soon though :). The collection APIs badly need a facelift

@jmesserly
Copy link

also there was a request for "removeItem", which I think means remove(T item), essentially removeAt(indexOf(item)) (ignoring the item not found check)

@lrhn
Copy link
Member

lrhn commented Sep 17, 2012

removeItem is dangerous in a List. It can mean either removeFirst or removeAll. I'd rather have both of those.

@jmesserly
Copy link

@LRN: totally agree

@alan-knight
Copy link
Contributor

Clarity is good, but so is brevity, so I dislike making all the names longer so we can remove all possible ambiguities. It might be sufficiently non-confusing if one of them was just called remove. e.g. if there's a remove() and a removeAll().

@munificent
Copy link
Member

+1 for remove()/removeAll().

It might be nice if remove() was in Collection so that you can have code that can generically remove from a List/Map/Set. In that case, removeFirst() isn't meaningful: what's the "first" matching item in a set?

@jmesserly
Copy link

That also matches what we do with "query" and "queryAll" from the DOM

@DartBot
Copy link

DartBot commented Sep 17, 2012

This comment was originally written by @seaneagan


I would say "removeAll" is the vast majority use case, and I could easily see someone assuming that "removeAll" takes a Collection of elements to remove from the receiver Collection. I think "removeFirst" is rare, and as jmesserly noted, easily written in terms of "indexOf", so I would leave it out, but if it's there, maybe call it "removeOne" in order to make it make sense for Sets as well.

@alan-knight
Copy link
Contributor

Interesting? I think that removeFirst is common and removeAll rare. Or, more particularly, I think that collections without duplicates is the most common case, so the two are equivalent and removeFirst is faster. And writing it in terms of indexOf every time is horrible.

@DartBot
Copy link

DartBot commented Sep 18, 2012

This comment was originally written by @seaneagan


I totally agree that Collections without duplicates are the most common,
but those really should be Sets or possibly OrderedSets, not Lists, in
which case "removeAll" can be optimized to behave the same as
"removeFirst"/"removeOne", so there is no need for both. In the case where
you do have duplicates, I think it's rare that you would want to remove *
only* the first instance, and not all. I do seem to be the outlier here
though :).

@lrhn
Copy link
Member

lrhn commented Sep 18, 2012

I can see that removeAll will be confusing when compared with Set.removeAll.
For consistency with Set, it might be better if "remove" would remove all instances of the element from the list, and a "removeFirst" (or similar) function could be used to remove only one instance (written in terms of indexOf and removeAt).

@DartBot
Copy link

DartBot commented Sep 18, 2012

This comment was originally written by @seaneagan


@LRN: sounds good to me.

A couple things to consider if including a List#removeFirst:

* should it take an optional "startIndex" parameter like "indexOf" does?

  • do we then need a corresponding "removeLast" to match "last", "lastIndexOf", "addLast"?

@DartBot
Copy link

DartBot commented Sep 18, 2012

This comment was originally written by @seaneagan


I just realized there is a different semantics for "removeFirst" proposed, see issue #604, so this one would might need to be "removeFirstOccurrence" or something.

@sethladd sethladd added Type-Enhancement area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core labels Sep 18, 2012
@sethladd sethladd added this to the Later milestone Sep 18, 2012
@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug and removed type-enhancement labels Mar 1, 2016
copybara-service bot pushed a commit that referenced this issue May 30, 2023
…buf, shelf, test, tools, vector_math, webdev

Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/59dc475..1d94484):
  1d94484c  2023-05-29  dependabot[bot]  Bump github/codeql-action from 2.3.3 to 2.3.5 (#3422)
  0edc1a71  2023-05-28  dependabot[bot]  Bump http from 0.13.6 to 1.0.0 (#3421)

http (https://github.com/dart-lang/http/compare/dfec389..8a4a4a6):
  8a4a4a6  2023-05-25  Brian Quinlan  Add a better toString to _ClientSocketException (#948)
  5c1f1ad  2023-05-25  Devon Carew  regenerate with the latest mono_repo (#947)

leak_tracker (https://github.com/dart-lang/leak_tracker/compare/9c6e9b3..7f2cab3):
  7f2cab3  2023-05-26  Polina Cherkasova  Nicely format retaining path. (#68)

lints (https://github.com/dart-lang/lints/compare/72f107a..4236c43):
  4236c43  2023-05-26  Parker Lougheed  Remove pedantic from README (#124)
  4ac79d8  2023-05-24  Parker Lougheed  Update example for latest lints version (#123)

mockito (https://github.com/dart-lang/mockito/compare/153c145..40fe2ca):
  40fe2ca  2023-05-25  Nate Bosch  Expand constraint on package:http

native (https://github.com/dart-lang/native/compare/45e16dc..76bc55e):
  76bc55e  2023-05-30  Daco Harkes  [c_compiler] Target ios_x64 (#53)

protobuf (https://github.com/dart-lang/protobuf/compare/7d2d293..346a72d):
  346a72d  2023-05-30  Ömer Sinan Ağacan  Fix generated ignore_for_file directives (#833)
  35ea45f  2023-05-26  Kevin Moore  Latest mono_repo (#834)

shelf (https://github.com/dart-lang/shelf/compare/56919a1..a404b6a):
  a404b6a  2023-05-25  Devon Carew  re-generate w/ the latest monorepo (#362)

test (https://github.com/dart-lang/test/compare/309596e..3276921):
  32769215  2023-05-25  dependabot[bot]  Bump github/codeql-action from 2.3.2 to 2.3.5 (#2023)
  f74e85c8  2023-05-25  dependabot[bot]  Bump dart-lang/setup-dart from 1.3.0 to 1.5.0 (#2022)
  4b2bd272  2023-05-25  Devon Carew  update the mono_repo and dependabot configs (#2021)

tools (https://github.com/dart-lang/tools/compare/81ff996..b90a7e8):
  b90a7e8  2023-05-26  Devon Carew  blast_repo fixes (#106)

vector_math (https://github.com/google/vector_math.dart/compare/e3de8da..cd87f57):
  cd87f57  2023-05-30  JKris95  Axis calculation of quaternions from small angles (#272)
  3762b25  2023-05-30  Lukas Klingsbo  Removes the `new` keyword from readme (#284)
  df5877f  2023-05-30  Lukas Klingsbo  Use named constructors in Vector2 and some general optimizations (#289)

webdev (https://github.com/dart-lang/webdev/compare/d74fadd..4b69f1d):
  4b69f1dd  2023-05-26  Anna Gringauze  fix format breaking tests (#2124)
  b75f8e62  2023-05-25  Devon Carew  re-generate w/ the latest monorepo (#2121)

Change-Id: Ide9b7781102b654db15114d01cd4fbca40478906
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/306304
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-core type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants