7
\$\begingroup\$

I've been suggested to put the following snippet of code up for review, hence I will do so, review of everything is appreciated, also posting as much relevant code as possible:

abstract public class Primitive {
    protected List<VertexData> vertexData;

    public List<VertexData> getVertexData() {
        return vertexData;
    }

    public static void calculateNormals(final List<Primitive> primitives) {
        primitives.stream()
                .flatMap(primitive -> primitive.getVertexData().stream())
                .collect(Collectors.groupingBy(VertexData::getVertex))      //Map<Vector3f, List<VertexData>>
                .entrySet().stream()
                .map(Map.Entry::getValue)                                   //List<VertexData>
                .forEach(Primitive::calculateNormalsOfVertexData);
    }

    private static void calculateNormalsOfVertexData(final List<VertexData> vertexData) {
        Vector3f averageNormal = vertexData.stream()
                .map(VertexData::getNormal)
                .reduce(new Vector3f().zero(), (n1, n2) -> n1.add(n2))
                .scale(1f / vertexData.size());
        vertexData.forEach(vd -> vd.setNormal(averageNormal));
    }
}

public class VertexData {
    private Vector3f vertex;
    private Vector3f normal;

    public VertexData(final Vector3f vertex, final Vector3f normal) {
        this.vertex = vertex;
        this.normal = normal;
    }

    public Vector3f getVertex() {
        return vertex;
    }

    public void setVertex(final Vector3f vertex) {
        this.vertex = vertex;
    }

    public Vector3f getNormal() {
        return normal;
    }

    public void setNormal(final Vector3f normal) {
        this.normal = normal;
    }

    @Override
    public String toString() {
        return "VertexData(" + vertex + ", " + normal + ")";
    }

    @Override
    public int hashCode() {
        int hash = 7;
        return hash;
    }

    @Override
    public boolean equals(Object obj) {
        if (obj == null) {
            return false;
        }
        if (getClass() != obj.getClass()) {
            return false;
        }
        final VertexData other = (VertexData) obj;
        if (!Objects.equals(this.vertex, other.vertex)) {
            return false;
        }
        if (!Objects.equals(this.normal, other.normal)) {
            return false;
        }
        return true;
    }
}

public class CustomCollectors {
    /**
     * Returns a mapping of an index to elements of the stream following the natural order at which the elements are to be encountered.
     * 
     * @param <T>   The type of th elements in the stream
     * @return  A Map<Long, T> that is indexed
     */
    public static <T> Collector<T, ?, Map<Long, T>> indexing() {
        return Collector.of(
                HashMap::new,
                (map, t) -> map.put(Long.valueOf(map.size() + 1), t),
                (left, right) -> {
                    final long size = left.size();
                    right.forEach((k, v) -> left.put(k + size, v));
                    return left;
                },
                Collector.Characteristics.CONCURRENT
        );
    }
}

Now onto the real code snippet that I would like to be reviewed, first of all what the code does is the following:

Given a List<Primitive>, I want to obtain an indexed Map<Long, VertexData> of the distinct VertexData of the Primitives.

My latest piece of code:

Map<Long, VertexData> vdMapping = primitives.stream()
        .flatMap(primitive -> primitive.getVertexData().stream())
        .distinct()
        .collect(CustomCollectors.indexing());
vdMapping.forEach((k, v) -> System.out.println("k = " + k + " / v = " + v));

Before the last refactoring:

AtomicLong index = new AtomicLong();
Map<Long, VertexData> vdMapping = primitives.stream()
        .flatMap(primitive -> primitive.getVertexData().stream())
        .distinct()
        .collect(Collectors.toMap(k -> index.getAndIncrement(), v -> v));
vdMapping.forEach((k, v) -> System.out.println("k = " + k + " / v = " + v));

And in comparison, how a Java 7 piece of code would look like:

Map<Long, VertexData> vdMapping = new HashMap<>();
Set<VertexData> vdSet = new HashSet<>();
long index = 0;
for (Primitive primitive : primitives) {
    for (VertexData vd : primitive.getVertexData()) {
        vdSet.add(vd);
    }
}
for (VertexData vd : vdSet) {
    vdMapping.put(index++, vd);
}
for (Map.Entry<Long, VertexData> entry : vdMapping.entrySet()) {
    System.out.println("k = " + entry.getKey() + " / v = " + entry.getValue());
}

A better name for CustomCollector.indexing() is also allowed.

\$\endgroup\$
  • 2
    \$\begingroup\$ Instead of looping and doing vdSet.add(vd); you can in Java 7 do vdSet.addAll(primitive.getVertexData()); \$\endgroup\$ – Simon Forsberg Mar 1 '15 at 13:23
4
\$\begingroup\$

long vs. int

In your map you are keyed off the long value of the index. The long is simply related to the size of the map as you accumulate things.

A Map can never have more than Integer.MAX_INT members, thus you can never accumulate more than that number of key values.... thus, why are you using a Long when an Integer will suffice?

Simplification

Consider this simplification, where the distinct phase is done in the intermediate Collector:

Map<Long, VertexData> vdata = primitives
    .stream()
    .collect(Collector.of(
         LinkedHashSet<VertexData>::new,
         (acc, t) -> acc.add(t),
         (left, right) -> {left.addAll(right); return left;}))
    .stream().collect(CustomCollectors.indexing());
vdMapping.forEach((k, v) -> System.out.println("k = " + k + " / v = " + v));

Conclusion

In this particular case, I am not certain that the streaming API from Java8 is the right solution. You are creating much more uncertainty than needs to happen.

The right solution to this is a LinkedHashSet data structure. Your Java7 equivalent is a better solution, but change the HashSet to a LinkedHashSet, and then your results are going to be deterministic. The code is simpler, more maintainable, and the parallel benefits of your stream are not possible anyway, so the added overheads of creating intermediate maps, running complicated distinct() filters, and other processes are just painful....

Just because it can be done with a stream does not mean that is the better solution.


Collector associativeness

One of the properties of a Collector is that it is supposed to be associative.

The associativity constraint says that splitting the computation must produce an equivalent result.

Your collector is not associative. Consider your implementation:

    return Collector.of(
            HashMap::new,
            (map, t) -> map.put(Long.valueOf(map.size() + 1), t),
            (left, right) -> {
                final long size = left.size();
                right.forEach((k, v) -> left.put(k + size, v));
                return left;
            },
            Collector.Characteristics.CONCURRENT
    );

If your collector is used on a concurrent stream, there is no way for you to determine the order of the combination function calls:

            (left, right) -> {
                final long size = left.size();
                right.forEach((k, v) -> left.put(k + size, v));
                return left;
            },

For example, suppose your stream is split in two, and the two parts are accumulated in two maps mapA and mapB. Your collector should produce the same results regardless of whether left == mapA && right == mapB or whether left == mapB && right == mapA.

Your streams will produce non-deterministic values for your inputs because your Collector is non-deterministic.

I am not certain how I would solve this problem.... there has to be a way to 'tag' the data at the beginning of the stream such that it is labelled with a 'key' sooner.... instead of arbitrarily, and non-deterministically assigning one later.

Conclusion

I believe the code produces results that are 'correct' for the context of the way you use it, but the results are non-deterministic, and thus are going to be a problem in the future when things go wrong.

You need to do something to fix the non-determinisim earlier in the stream:

Map<Long, VertexData> vdMapping = primitives.stream()
        .flatMap(primitive -> primitive.getVertexData().stream())
        .distinct()
        .collect(CustomCollectors.indexing());
vdMapping.forEach((k, v) -> System.out.println("k = " + k + " / v = " + v));

I believe the only decent solution would be to do something like guarantee sequential processing until the data leaves the distinct() phase, and then add a map phase at that point which numbers the VertexData that exits at that point.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.