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 top-level combine<Iterables|Lists|Maps> #711

Closed
matanlurey opened this issue Jan 20, 2017 · 4 comments · Fixed by dart-archive/collection#53
Closed

Add top-level combine<Iterables|Lists|Maps> #711

matanlurey opened this issue Jan 20, 2017 · 4 comments · Fixed by dart-archive/collection#53
Assignees
Labels
package:collection type-enhancement A request for a change that isn't a bug

Comments

@matanlurey
Copy link
Contributor

Let's start with just discussing combineList, which is currently implemented in various forms internally, and has lots of utility and potential perf-saving.

//// Returns a new list that represents [lists] flattened into a single list.
///
/// All methods and accessors treat the new list as-if it were a single concatenated list,
/// but the underlying implementation delegates to each corresponding list as necessary. 
List<T> combineLists<T>(List<List<T>> lists) => ...

Here is an example of implementing first:

class _CombinedList<T> implements List<T> {
  final List<T> _lists;

  T get first {
    for (final list in _lists) {
     if (list.isNotEmpty) {
       return list.first; 
     }
    }
   throw ...
  }
}

/cc @nex3, @leonsenft, @ferhatb

FWIW I believe @isoos originally implemented something like this for the high-performance table, and it worked really well. I'd also eventually like to see the same thing for Map and Iterable if possible - though to simplify things they may need to be read-only, but worth discussing anyway.

@nex3
Copy link
Member

nex3 commented Jan 20, 2017

I could see a CombinedListView class for this. Does this mean that [] would be O(n) for the number of lists that were combined? Or would you track some sort of radix list?

@nex3 nex3 added the type-enhancement A request for a change that isn't a bug label Jan 20, 2017
@matanlurey
Copy link
Contributor Author

I could see a CombinedListView class for this. Does this mean that [] would be O(n) for the number of lists that were combined? Or would you track some sort of radix list?

I think given that I imagine this is usually just 2 or 3 lists, I'd probably implement as a O(n) where n is the number of lists that were combined. That's how it is implemented today and is definitely just a few lines of code.

@nex3
Copy link
Member

nex3 commented Jan 20, 2017

That sounds fine to me.

@matanlurey matanlurey self-assigned this Jan 20, 2017
@matanlurey matanlurey changed the title FR: Add top-level functions combine<Iterables|Lists|Maps> Add top-level combine<Iterables|Lists|Maps> Jan 20, 2017
@isoos
Copy link

isoos commented Jan 20, 2017

I don't think O(number of lists|maps combined) is a big issue. When people want to combine 1000s of lists or maps, they will probably figure out better data structures to represent what they need anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:collection type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants