Either is fine. Pick the one that seems more readable to you. If the calculation naturally decomposes, as this one does, then the multiple maps is probably more readable. Some calculations won't naturally decompose, in which case you're stuck at the former. In neither case should you be worrying that one is significantly more performant than the other; that's largely a non-consideration.
Answer from Brian Goetz on Stack OverflowEither is fine. Pick the one that seems more readable to you. If the calculation naturally decomposes, as this one does, then the multiple maps is probably more readable. Some calculations won't naturally decompose, in which case you're stuck at the former. In neither case should you be worrying that one is significantly more performant than the other; that's largely a non-consideration.
use filter before map.
data.stream().filter(Objects::nonNull).
.map(x -> x*x)
filter(Objects::nonNull) try this instead .
WARNING
As mentioned in comments, Using peek() for production code is considered bad practice
The reasson is that "According to its JavaDocs, the intermediate Stream operation java.util.Stream.peek() “exists mainly to support debugging” purposes."
As a consequence, this proposed solution SHOULD NOT be used.
Forgot to relate to the first code snippet. I wouldn't use forEach at all. Since you are collecting the elements of the Stream into a List, it would make more sense to end the Stream processing with collect. Then you would need peek in order to set the ID.
List<Entry> updatedEntries =
entryList.stream()
.peek(e -> e.setTempId(tempId))
.collect (Collectors.toList());
For the second snippet, forEach can execute multiple expressions, just like any lambda expression can :
entryList.forEach(entry -> {
if(entry.getA() == null){
printA();
}
if(entry.getB() == null){
printB();
}
if(entry.getC() == null){
printC();
}
});
However (looking at your commented attempt), you can't use filter in this scenario, since you will only process some of the entries (for example, the entries for which entry.getA() == null) if you do.
List<String> items = new ArrayList<>();
items.add("A");
items.add("B");
items.add("C");
items.add("D");
items.add("E");
//lambda
//Output : A,B,C,D,E
items.forEach(item->System.out.println(item));
//Output : C
items.forEach(item->{
System.out.println(item);
System.out.println(item.toLowerCase());
}
});
Instead of replacing the space padding you could use BigInteger and prepend a '0' during the format step. This works because BigInteger is treated as integral.
String bin = str.chars()
.mapToObj(Integer::toBinaryString)
.map(BigInteger::new)
.map(x -> String.format("%08d", x))
.collect(Collectors.joining(" "));
EDIT
As @Holger suggested there is a more lightweight solution using long instead of BigInteger, since the binary representation of Character.MAX_VALUE does not exceed the limit of long.
String bin = str.chars()
.mapToObj(Integer::toBinaryString)
.mapToLong(Long::parseLong)
.mapToObj(l -> String.format("%08d", l))
.collect(Collectors.joining(" "));
.mapToObj(x-> {
String s = Integer.toBinaryString(x);
s = String.format("%8s", s).replaceAll(" ", "0");
return s;
})
That depends on how you define better. Better might be more CPU or memory efficient, or it could be fewer bytes in the compiled code, or better parallelization.
But I am going to take a chance on assuming that this code runs on a reasonably normal platform and has no special requirements, and therefore being better generally means better readability.
Readability Improvements
In which case, your code is fine. But we can make it easier to read by splitting it into named steps and wrapping it in a good function.
public List<Integer> getNumbersOccuringOddTimes(final List<Integer> numbers) {
final Map<Integer, Integer> numberCounts = numbers.stream().collect(groupingBy(Function.identity(), counting()));
final Predicate<Entry> isOdd = e -> e.getValue() % 2 == 1;
return numberCounts.entrySet().stream().filter(isOdd).map(Entry::getKey).collect(toList());
}
I've made five minor adjustments to your code to help with readability. I have;
- Added a descriptive method name that wraps the steps.
- Split the two streams into separate statements.
- Used static imports (not shown) for the Collectors functions.
- Moved the modulus check into a predicate variable (
isOdd) so it can be named. - Swapped out the lambda in
.mapto a method reference because it is shorter and more descriptive.
As Rudi Kershaw said, there isn't much to fix technically. Whatever your way of implementation is, you need to count the occurences, select the odd ones and collect the values. Streams are not inherently readable so what you can do is to concentrate on the presentation a bit more. The variable names you use are either straight out confusing or just not very descriptive.
For example, you're processing Map.Entry objects, not Map objects, when filtering out the odd ones. Thus use entry in the lambda instead of map:
.filter(entry -> entry.getValue() % 2 == 1)
Likewise when picking the keys from the entries, use a variable name that describes the object being processed:
.map(entry -> entry.getKey())
With those changes and some formatting, your code becomes this. For someone who is familiar with common stream concepts, this should be pretty clear.
List<Integer> oddOcuranceNumbers = numbers.stream()
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()))
.entrySet().stream()
.filter(entry -> entry.getValue() % 2 == 1)
.map(entry -> entry.getKey())
.collect(Collectors.toList());
You may do it like so,
List<String> interchanged = cs.stream()
.map(s -> s.equals("manager") ? "agent" : s.equals("agent") ? "manager" : s)
.collect(Collectors.toList());
Another way to do that using List.replaceAll could be:
List<String> cs = Arrays.asList("agent", "manager", "admin");
cs.replaceAll(s -> {
if (s.equals("manager")) {
return "agent";
}
if (s.equals("agent")) {
return "manager";
}
return s;
});
Easiest way is to write your own lambda expression that does the conversion to uppercase for you:
List<String> firstNames = customers.stream()
.map(customer->StringUtils.uppercase(customer.getFirstName()))
.collect(Collectors.toList());
Even in your code you are taking the customer name and then doing toUpperCase.
List<String> firstNames = customers.stream()
.map(customer-> customer.getFirstName())
.map(firstName -> StringUtils.uppercase(firstName))
.collect(Collectors.toList())
The problem comes from the fact that you're using a generic wildcard ?. What you want is to have a parameterized type T, that will represent the type of the Stream element. Assuming the function would return the same type as their input, you could have:
private static <T> List<T> multipleMapping(final Collection<T> collection, final List<Function<T, T>> functions) {
Stream<T> stream = collection.stream();
for (Function<T, T> function : functions) {
stream = stream.map(function);
}
return stream.collect(Collectors.toList());
}
This compiles fine: the mapper given to map correcly accepts a T and returns a T. However, if the functions don't return the same type as their input then you won't be able to keep type-safety and will have to resort to using List<Function<Object, Object>>.
Note that we could use a UnaryOperator<T> instead of Function<T, T>.
Also, you could avoid the for loop and reduce all functions into a single one using andThen:
private static <T> List<T> multipleMapping(final Collection<T> collection, final List<Function<T, T>> functions) {
return collection.stream()
.map(functions.stream().reduce(Function.identity(), Function::andThen))
.collect(Collectors.toList());
}
If you have few functions (i.e. if you can write them down), then I suggest you don't add them to a list. Instead, compose them into a single function, and then apply that single function to each element of the given collection.
Your multipleMapping() method would now receive a single function:
public static <T, R> List<R> multipleMapping(
Collection<T> collection, Function<T, R> function) {
return collection.stream()
.map(function)
.collect(Collectors.toList());
}
Then, in the calling code, you could create a function composed of many functions (you will have all the functions anyway) and invoke the multipleMapping() method with that function.
For example, suppose we have a list of candidates:
List<String> candidates = Arrays.asList(
"Hillary", "Donald",
"Bernie", "Ted", "John");
And four functions:
Function<String, Integer> f1 = String::length;
Function<Integer, Long> f2 = i -> i * 10_000L;
Function<Long, LocalDate> f3 = LocalDate::ofEpochDay;
Function<LocalDate, Integer> f4 = LocalDate::getYear;
These functions can be used to compose a new function, as follows:
Function<String, Integer> function = f1.andThen(f2).andThen(f3).andThen(f4);
Or also this way:
Function<String, Integer> composed = f4.compose(f3).compose(f2).compose(f1);
Now, you can invoke your multipleMapping() method with the list of candidates and the composed function:
List<Integer> scores = multipleMapping(candidates, function);
So we have transformed our list of candidates into a list of scores, by explicitly composing a new function from four different functions and applying this composed function to each candidate.
If you want to know who will win the election, you could check which candidate has the highest score, but I will let that as an exercise for whoever is interested in politics ;)
It's an interesting question, because it shows that there are a lot of different approaches to achieve the same result. Below I show three different implementations.
Default methods in Collection Framework: Java 8 added some methods to the collections classes, that are not directly related to the Stream API. Using these methods, you can significantly simplify the implementation of the non-stream implementation:
Collection<DataSet> convert(List<MultiDataPoint> multiDataPoints) {
Map<String, DataSet> result = new HashMap<>();
multiDataPoints.forEach(pt ->
pt.keyToData.forEach((key, value) ->
result.computeIfAbsent(
key, k -> new DataSet(k, new ArrayList<>()))
.dataPoints.add(new DataPoint(pt.timestamp, value))));
return result.values();
}
Stream API with flatten and intermediate data structure: The following implementation is almost identical to the solution provided by Stuart Marks. In contrast to his solution, the following implementation uses an anonymous inner class as intermediate data structure.
Collection<DataSet> convert(List<MultiDataPoint> multiDataPoints) {
return multiDataPoints.stream()
.flatMap(mdp -> mdp.keyToData.entrySet().stream().map(e ->
new Object() {
String key = e.getKey();
DataPoint dataPoint = new DataPoint(mdp.timestamp, e.getValue());
}))
.collect(
collectingAndThen(
groupingBy(t -> t.key, mapping(t -> t.dataPoint, toList())),
m -> m.entrySet().stream().map(e -> new DataSet(e.getKey(), e.getValue())).collect(toList())));
}
Stream API with map merging: Instead of flattening the original data structures, you can also create a Map for each MultiDataPoint, and then merge all maps into a single map with a reduce operation. The code is a bit simpler than the above solution:
Collection<DataSet> convert(List<MultiDataPoint> multiDataPoints) {
return multiDataPoints.stream()
.map(mdp -> mdp.keyToData.entrySet().stream()
.collect(toMap(e -> e.getKey(), e -> asList(new DataPoint(mdp.timestamp, e.getValue())))))
.reduce(new HashMap<>(), mapMerger())
.entrySet().stream()
.map(e -> new DataSet(e.getKey(), e.getValue()))
.collect(toList());
}
You can find an implementation of the map merger within the Collectors class. Unfortunately, it is a bit tricky to access it from the outside. Following is an alternative implementation of the map merger:
<K, V> BinaryOperator<Map<K, List<V>>> mapMerger() {
return (lhs, rhs) -> {
Map<K, List<V>> result = new HashMap<>();
lhs.forEach((key, value) -> result.computeIfAbsent(key, k -> new ArrayList<>()).addAll(value));
rhs.forEach((key, value) -> result.computeIfAbsent(key, k -> new ArrayList<>()).addAll(value));
return result;
};
}
To do this, I had to come up with an intermediate data structure:
class KeyDataPoint {
String key;
DateTime timestamp;
Number data;
// obvious constructor and getters
}
With this in place, the approach is to "flatten" each MultiDataPoint into a list of (timestamp, key, data) triples and stream together all such triples from the list of MultiDataPoint.
Then, we apply a groupingBy operation on the string key in order to gather the data for each key together. Note that a simple groupingBy would result in a map from each string key to a list of the corresponding KeyDataPoint triples. We don't want the triples; we want DataPoint instances, which are (timestamp, data) pairs. To do this we apply a "downstream" collector of the groupingBy which is a mapping operation that constructs a new DataPoint by getting the right values from the KeyDataPoint triple. The downstream collector of the mapping operation is simply toList which collects the DataPoint objects of the same group into a list.
Now we have a Map<String, List<DataPoint>> and we want to convert it to a collection of DataSet objects. We simply stream out the map entries and construct DataSet objects, collect them into a list, and return it.
The code ends up looking like this:
Collection<DataSet> convertMultiDataPointToDataSet(List<MultiDataPoint> multiDataPoints) {
return multiDataPoints.stream()
.flatMap(mdp -> mdp.getData().entrySet().stream()
.map(e -> new KeyDataPoint(e.getKey(), mdp.getTimestamp(), e.getValue())))
.collect(groupingBy(KeyDataPoint::getKey,
mapping(kdp -> new DataPoint(kdp.getTimestamp(), kdp.getData()), toList())))
.entrySet().stream()
.map(e -> new DataSet(e.getKey(), e.getValue()))
.collect(toList());
}
I took some liberties with constructors and getters, but I think they should be obvious.
Generally collecting to anything other than standard API's gives you is pretty easy via a custom Collector. In your case collecting to 3 lists at a time (just a small example that compiles, since you can't share your code either):
private static <T> Collector<T, ?, List<List<T>>> to3Lists() {
class Acc {
List<T> left = new ArrayList<>();
List<T> middle = new ArrayList<>();
List<T> right = new ArrayList<>();
List<List<T>> list = Arrays.asList(left, middle, right);
void add(T elem) {
// obviously do whatever you want here
left.add(elem);
middle.add(elem);
right.add(elem);
}
Acc merge(Acc other) {
left.addAll(other.left);
middle.addAll(other.middle);
right.addAll(other.right);
return this;
}
public List<List<T>> finisher() {
return list;
}
}
return Collector.of(Acc::new, Acc::add, Acc::merge, Acc::finisher);
}
And using it via:
Stream.of(1, 2, 3)
.collect(to3Lists());
Obviously this custom collector does not do anything useful, but just an example of how you could work with it.
I have adapted the answer to this question to your case. The custom Spliterator will "split" the stream into multiple streams that collect by different properties:
@SafeVarargs
public static <T> long streamForked(Stream<T> source, Consumer<Stream<T>>... consumers)
{
return StreamSupport.stream(new ForkingSpliterator<>(source, consumers), false).count();
}
public static class ForkingSpliterator<T>
extends AbstractSpliterator<T>
{
private Spliterator<T> sourceSpliterator;
private List<BlockingQueue<T>> queues = new ArrayList<>();
private boolean sourceDone;
@SafeVarargs
private ForkingSpliterator(Stream<T> source, Consumer<Stream<T>>... consumers)
{
super(Long.MAX_VALUE, 0);
sourceSpliterator = source.spliterator();
for (Consumer<Stream<T>> fork : consumers)
{
LinkedBlockingQueue<T> queue = new LinkedBlockingQueue<>();
queues.add(queue);
new Thread(() -> fork.accept(StreamSupport.stream(new ForkedConsumer(queue), false))).start();
}
}
@Override
public boolean tryAdvance(Consumer<? super T> action)
{
sourceDone = !sourceSpliterator.tryAdvance(t -> queues.forEach(queue -> queue.offer(t)));
return !sourceDone;
}
private class ForkedConsumer
extends AbstractSpliterator<T>
{
private BlockingQueue<T> queue;
private ForkedConsumer(BlockingQueue<T> queue)
{
super(Long.MAX_VALUE, 0);
this.queue = queue;
}
@Override
public boolean tryAdvance(Consumer<? super T> action)
{
while (queue.peek() == null)
{
if (sourceDone)
{
// element is null, and there won't be no more, so "terminate" this sub stream
return false;
}
}
// push to consumer pipeline
action.accept(queue.poll());
return true;
}
}
}
You can use it as follows:
streamForked(Stream.of(new Row("content1", "client1", "location1", 1),
new Row("content2", "client1", "location1", 2),
new Row("content1", "client1", "location2", 3),
new Row("content2", "client2", "location2", 4),
new Row("content1", "client2", "location2", 5)),
rows -> System.out.println(rows.collect(Collectors.groupingBy(Row::getClient,
Collectors.groupingBy(Row::getContent,
Collectors.summingInt(Row::getConsumption))))),
rows -> System.out.println(rows.collect(Collectors.groupingBy(Row::getClient,
Collectors.groupingBy(Row::getLocation,
Collectors.summingInt(Row::getConsumption))))),
rows -> System.out.println(rows.collect(Collectors.groupingBy(Row::getContent,
Collectors.groupingBy(Row::getLocation,
Collectors.summingInt(Row::getConsumption))))));
// Output
// {client2={location2=9}, client1={location1=3, location2=3}}
// {client2={content2=4, content1=5}, client1={content2=2, content1=4}}
// {content2={location1=2, location2=4}, content1={location1=1, location2=8}}
Note that you can do pretty much anything you want with your the copies of the stream. As per your example, I used a stacked groupingBy collector to group the rows by two properties and then summed up the int property. So the result will be a Map<String, Map<String, Integer>>. But you could also use it for other scenarios:
rows -> System.out.println(rows.count())
rows -> rows.forEach(row -> System.out.println(row))
rows -> System.out.println(rows.anyMatch(row -> row.getConsumption() > 3))
If you just want to filter out the values that are Boolean and present in the pairs map, you can apply filter function:
Stream.of(values).filter(value -> pairs.get(value) != null && pairs.get(value) instanceof Boolean)
Or if you want to actually return true and false values, you can use map:
return Stream.of(values).allMatch(value -> {
if (pairs.get(value) == null) {
return false;
}
if ((pairs.get(value) instanceof Boolean)) {
return true;
}
errors.add(value + " does not contain a valid boolean value");
return false;
});
I think you need Just :
return Arrays.stream(values)
.allMatch(value -> pairs.get(value) instanceof Boolean);
Notes
- I don't see any reason of
errorin your method - also as @Andy Turner mention in comment
pairs.get(value) instanceof Booleanimpliespairs.get(value) != nullso you don't need to usepairs.get(value) != null