#304 Addressing review remarks

This commit is contained in:
Gunnar Morling 2015-03-03 22:57:18 +01:00
parent 21b89ea5e2
commit b02d206de6
3 changed files with 101 additions and 93 deletions

View File

@ -41,6 +41,7 @@ import org.mapstruct.ap.model.PropertyMapping.PropertyMappingBuilder;
import org.mapstruct.ap.model.common.Parameter; import org.mapstruct.ap.model.common.Parameter;
import org.mapstruct.ap.model.common.Type; import org.mapstruct.ap.model.common.Type;
import org.mapstruct.ap.model.dependency.GraphAnalyzer; import org.mapstruct.ap.model.dependency.GraphAnalyzer;
import org.mapstruct.ap.model.dependency.GraphAnalyzer.GraphAnalyzerBuilder;
import org.mapstruct.ap.model.source.Mapping; import org.mapstruct.ap.model.source.Mapping;
import org.mapstruct.ap.model.source.SourceMethod; import org.mapstruct.ap.model.source.SourceMethod;
import org.mapstruct.ap.model.source.SourceReference; import org.mapstruct.ap.model.source.SourceReference;
@ -175,16 +176,16 @@ public class BeanMappingMethod extends MappingMethod {
* detected, an error is reported. * detected, an error is reported.
*/ */
private void sortPropertyMappingsByDependencies() { private void sortPropertyMappingsByDependencies() {
final GraphAnalyzer graphAnalyzer = new GraphAnalyzer(); GraphAnalyzerBuilder graphAnalyzerBuilder = GraphAnalyzer.builder();
for ( PropertyMapping propertyMapping : propertyMappings ) { for ( PropertyMapping propertyMapping : propertyMappings ) {
graphAnalyzer.addNode( propertyMapping.getName(), propertyMapping.getDependsOn() ); graphAnalyzerBuilder.withNode( propertyMapping.getName(), propertyMapping.getDependsOn() );
} }
graphAnalyzer.analyze(); final GraphAnalyzer graphAnalyzer = graphAnalyzerBuilder.build();
if ( !graphAnalyzer.getCycles().isEmpty() ) { if ( !graphAnalyzer.getCycles().isEmpty() ) {
Set<String> cycles = new HashSet<String>( graphAnalyzer.getCycles().size() ); Set<String> cycles = new HashSet<String>();
for ( List<String> cycle : graphAnalyzer.getCycles() ) { for ( List<String> cycle : graphAnalyzer.getCycles() ) {
cycles.add( Strings.join( cycle, " -> " ) ); cycles.add( Strings.join( cycle, " -> " ) );
} }

View File

@ -19,6 +19,7 @@
package org.mapstruct.ap.model.dependency; package org.mapstruct.ap.model.dependency;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections; import java.util.Collections;
import java.util.HashMap; import java.util.HashMap;
import java.util.HashSet; import java.util.HashSet;
@ -34,32 +35,29 @@ import java.util.Stack;
*/ */
public class GraphAnalyzer { public class GraphAnalyzer {
private final Map<String, Node> nodes = new HashMap<String, Node>(); private final Map<String, Node> nodes;
private final Set<List<String>> cycles = new HashSet<List<String>>(); private final Set<List<String>> cycles;
private final Stack<Node> currentPath;
private final Stack<Node> currentPath = new Stack<Node>(); private GraphAnalyzer(Map<String, Node> nodes) {
this.nodes = nodes;
public void addNode(String name, List<String> descendants) { cycles = new HashSet<List<String>>();
Node node = getNode( name ); currentPath = new Stack<Node>();
for ( String descendant : descendants ) {
node.addDescendant( getNode( descendant ) );
}
} }
public void addNode(String name, String... descendants) { public static GraphAnalyzerBuilder builder() {
Node node = getNode( name ); return new GraphAnalyzerBuilder();
}
for ( String descendant : descendants ) { public static GraphAnalyzerBuilder withNode(String name, String... descendants) {
node.addDescendant( getNode( descendant ) ); return builder().withNode( name, descendants );
}
} }
/** /**
* Performs a full traversal of the graph, detecting potential cycles and calculates the full list of descendants of * Performs a full traversal of the graph, detecting potential cycles and calculates the full list of descendants of
* the nodes. * the nodes.
*/ */
public void analyze() { private void analyze() {
for ( Node node : nodes.values() ) { for ( Node node : nodes.values() ) {
depthFirstSearch( node ); depthFirstSearch( node );
} }
@ -109,7 +107,7 @@ public class GraphAnalyzer {
boolean inCycle = false; boolean inCycle = false;
for ( Node n : currentPath ) { for ( Node n : currentPath ) {
if ( n.getName().equals( start.getName() ) ) { if ( !inCycle && n.equals( start ) ) {
inCycle = true; inCycle = true;
} }
@ -121,14 +119,43 @@ public class GraphAnalyzer {
return cycle; return cycle;
} }
private Node getNode(String name) { public static class GraphAnalyzerBuilder {
Node node = nodes.get( name );
if ( node == null ) { private final Map<String, Node> nodes = new HashMap<String, Node>();
node = new Node( name );
nodes.put( name, node ); public GraphAnalyzerBuilder withNode(String name, List<String> descendants) {
Node node = getNode( name );
for ( String descendant : descendants ) {
node.addDescendant( getNode( descendant ) );
}
return this;
} }
return node; public GraphAnalyzerBuilder withNode(String name, String... descendants) {
return withNode( name, Arrays.asList( descendants ) );
}
/**
* Builds the analyzer and triggers traversal of all nodes for detecting potential cycles and calculates the
* full list of descendants of each node.
*/
public GraphAnalyzer build() {
GraphAnalyzer graphAnalyzer = new GraphAnalyzer( nodes );
graphAnalyzer.analyze();
return graphAnalyzer;
}
private Node getNode(String name) {
Node node = nodes.get( name );
if ( node == null ) {
node = new Node( name );
nodes.put( name, node );
}
return node;
}
} }
} }

View File

@ -25,7 +25,6 @@ import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Set; import java.util.Set;
import org.junit.Before;
import org.junit.Test; import org.junit.Test;
import org.mapstruct.ap.model.dependency.GraphAnalyzer; import org.mapstruct.ap.model.dependency.GraphAnalyzer;
import org.mapstruct.ap.util.Strings; import org.mapstruct.ap.util.Strings;
@ -37,24 +36,15 @@ import org.mapstruct.ap.util.Strings;
*/ */
public class GraphAnalyzerTest { public class GraphAnalyzerTest {
private GraphAnalyzer detector;
@Before
public void setUpDetector() {
detector = new GraphAnalyzer();
}
@Test @Test
public void emptyGraph() { public void emptyGraph() {
detector.analyze(); GraphAnalyzer detector = GraphAnalyzer.builder().build();
assertThat( detector.getCycles() ).isEmpty(); assertThat( detector.getCycles() ).isEmpty();
} }
@Test @Test
public void singleNode() { public void singleNode() {
detector.addNode( "a" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a" ).build();
detector.analyze();
assertThat( detector.getCycles() ).isEmpty(); assertThat( detector.getCycles() ).isEmpty();
assertThat( detector.getAllDescendants( "a" ) ).isEmpty(); assertThat( detector.getAllDescendants( "a" ) ).isEmpty();
@ -62,10 +52,9 @@ public class GraphAnalyzerTest {
@Test @Test
public void twoNodesWithoutCycle() { public void twoNodesWithoutCycle() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b" ); .withNode( "b" )
.build();
detector.analyze();
assertThat( detector.getCycles() ).isEmpty(); assertThat( detector.getCycles() ).isEmpty();
assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b" ); assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b" );
@ -74,31 +63,28 @@ public class GraphAnalyzerTest {
@Test @Test
public void twoNodesWithCycle() { public void twoNodesWithCycle() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b", "a" ); .withNode( "b", "a" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a" ); assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a" );
} }
@Test @Test
public void threeNodesWithCycleBetweenTwo() { public void threeNodesWithCycleBetweenTwo() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b", "a", "c" ); .withNode( "b", "a", "c" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a" ); assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a" );
} }
@Test @Test
public void twoNodesWithSharedDescendantWithoutCycle() { public void twoNodesWithSharedDescendantWithoutCycle() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b", "c" ); .withNode( "b", "c" )
detector.addNode( "a", "c" ); .withNode( "a", "c" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).isEmpty(); assertThat( asStrings( detector.getCycles() ) ).isEmpty();
assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b", "c" ); assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b", "c" );
@ -108,10 +94,9 @@ public class GraphAnalyzerTest {
@Test @Test
public void threeNodesWithoutCycle() { public void threeNodesWithoutCycle() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "c", "b" ); .withNode( "c", "b" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).isEmpty(); assertThat( asStrings( detector.getCycles() ) ).isEmpty();
@ -122,36 +107,33 @@ public class GraphAnalyzerTest {
@Test @Test
public void fourNodesWithCycleBetweenThree() { public void fourNodesWithCycleBetweenThree() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b", "c" ); .withNode( "b", "c" )
detector.addNode( "c", "d" ); .withNode( "c", "d" )
detector.addNode( "d", "b" ); .withNode( "d", "b" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).containsOnly( "b -> c -> d -> b" ); assertThat( asStrings( detector.getCycles() ) ).containsOnly( "b -> c -> d -> b" );
} }
@Test @Test
public void fourNodesWithTwoCycles() { public void fourNodesWithTwoCycles() {
detector.addNode( "a", "b" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b" )
detector.addNode( "b", "a" ); .withNode( "b", "a" )
detector.addNode( "c", "d" ); .withNode( "c", "d" )
detector.addNode( "d", "c" ); .withNode( "d", "c" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a", "c -> d -> c" ); assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b -> a", "c -> d -> c" );
} }
@Test @Test
public void fourNodesWithoutCycle() { public void fourNodesWithoutCycle() {
detector.addNode( "a", "b1" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b1" )
detector.addNode( "a", "b2" ); .withNode( "a", "b2" )
detector.addNode( "b1", "c" ); .withNode( "b1", "c" )
detector.addNode( "b2", "c" ); .withNode( "b2", "c" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).isEmpty(); assertThat( asStrings( detector.getCycles() ) ).isEmpty();
assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b1", "b2", "c" ); assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b1", "b2", "c" );
@ -162,27 +144,25 @@ public class GraphAnalyzerTest {
@Test @Test
public void fourNodesWithCycle() { public void fourNodesWithCycle() {
detector.addNode( "a", "b1" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b1" )
detector.addNode( "a", "b2" ); .withNode( "a", "b2" )
detector.addNode( "b1", "c" ); .withNode( "b1", "c" )
detector.addNode( "b2", "c" ); .withNode( "b2", "c" )
detector.addNode( "c", "a" ); .withNode( "c", "a" )
.build();
detector.analyze();
assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b1 -> c -> a", "a -> b2 -> c -> a" ); assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b1 -> c -> a", "a -> b2 -> c -> a" );
} }
@Test @Test
public void eightNodesWithoutCycle() { public void eightNodesWithoutCycle() {
detector.addNode( "a", "b1" ); GraphAnalyzer detector = GraphAnalyzer.withNode( "a", "b1" )
detector.addNode( "a", "b2" ); .withNode( "a", "b2" )
detector.addNode( "b1", "c1" ); .withNode( "b1", "c1" )
detector.addNode( "b1", "c2" ); .withNode( "b1", "c2" )
detector.addNode( "b2", "c3" ); .withNode( "b2", "c3" )
detector.addNode( "b2", "c4" ); .withNode( "b2", "c4" )
.build();
detector.analyze();
assertThat( detector.getCycles() ).isEmpty(); assertThat( detector.getCycles() ).isEmpty();