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

[Issue#9] - internal/graph: partition algorithm #16

Draft
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

mccurdyc
Copy link
Owner

Proposed Changes:

Relevant Issue(s)/Link(s):

  • Fixes #

Describe Your Testing Method(s):

  • go test ./...

Risks/Caveats:

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
…connectedness

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
… distance

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
internal/graph/graph.go Outdated Show resolved Hide resolved
…global) vs just min (local) of connectedness value

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
…ds to funcs

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
… against edge distance

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := test.graph.FindRoots()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope test in function literal (from scopelint)

t.Run(test.name, func(t *testing.T) {
got := test.graph.FindRoots()

if diff := cmp.Diff(test.want, got); diff != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope test in function literal (from scopelint)

got := test.graph.FindRoots()

if diff := cmp.Diff(test.want, got); diff != "" {
t.Errorf("(%+v) FindRoots() mismatch (-want +got): \n%s", test.graph, diff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope test in function literal (from scopelint)

@@ -31,15 +31,23 @@ func TestNew(t *testing.T) {
}

var (
parentEdge = map[string]WeightedEdge{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parentEdge is a global variable (from gochecknoglobals)

edgeB = map[string]WeightedEdge{
"b": WeightedEdge{
Weight: 1.0,
Dest: &Node{ID: "b", Edges: make(map[string]WeightedEdge)},
},
}

nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeAWithSingleEdgeB is a global variable (from gochecknoglobals)

nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeCWithSingleEdgeB is a global variable (from gochecknoglobals)

nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeCWithEdgeBAndParent = Node{ID: "c", Edges: edgeB, Parents: parentEdge}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeCWithEdgeBAndParent is a global variable (from gochecknoglobals)

@@ -5,6 +5,26 @@ import (
"testing"
)

var (
nodeA = Node{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeA is a global variable (from gochecknoglobals)

ShortestPathLen: 1.0,
}

nodeB = Node{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeB is a global variable (from gochecknoglobals)

func (g Graph) FindRoots() []*Node {
roots := make([]*Node, 0, len(g))

for _, node := range g {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

order matters here

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
o := mockObject{
pkgFn: tt.pkgFn,
nameFn: tt.nameFn,
posFn: tt.posFn,
typeFn: tt.typeFn,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope tt in function literal (from scopelint)

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := test.graph.Roots()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope test in function literal (from scopelint)

got := test.graph.Roots()

if diff := cmp.Diff(test.want, got); diff != "" {
t.Errorf("(%+v) Roots() mismatch (-want +got): \n%s", test.graph, diff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope test in function literal (from scopelint)

@@ -38,7 +68,7 @@ func TestAddEdge(t *testing.T) {
tt.node.AddEdge(tt.dest, tt.weight)

if ok := reflect.DeepEqual(*tt.node, tt.want); !ok {
t.Errorf("(%+v) AddEdge(%+v, %f) - mismatch \n%+v", tt.node, tt.dest, tt.weight, tt.want)
t.Errorf("AddEdge() - mismatch \n\twant: %+v\n\tgot:%+v", tt.want, *tt.node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope tt in function literal (from scopelint)

@@ -31,15 +31,25 @@ func TestNew(t *testing.T) {
}

var (
parentNode = Node{ID: "parent", Edges: make(map[string]WeightedEdge), Parents: make(map[string]WeightedEdge)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parentNode is a global variable (from gochecknoglobals)

},
}

nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB, Parents: make(map[string]WeightedEdge)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeAWithSingleEdgeB is a global variable (from gochecknoglobals)

nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB}
nodeAWithSingleEdgeB = Node{ID: "a", Edges: edgeB, Parents: make(map[string]WeightedEdge)}
nodeCWithSingleEdgeB = Node{ID: "c", Edges: edgeB, Parents: make(map[string]WeightedEdge)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nodeCWithSingleEdgeB is a global variable (from gochecknoglobals)

)

var (
emptyNode = graph.Node{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emptyNode is a global variable (from gochecknoglobals)

…ren't added

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
…eing evaluated. this race condition lead to missing edges if method nodes were added first

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
… terminate recursion! It completes now

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
… partitions

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
…owest). add a log message and avoid division by 0

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

// recursiveBFS does recursive breadth-first search of the graph, keeping track
// of shortest path and the mininimum path strength (i.e., the weaked edge in a path).
func recursiveBFS(node *Node, visited map[string]bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cyclomatic complexity 12 of func recursiveBFS is high (> 10) (from gocyclo)

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

// recursiveBFS does recursive breadth-first search of the graph, keeping track
// of shortest path and the mininimum path strength (i.e., the weaked edge in a path).
func recursiveBFS(level map[string]*Node, visited map[string]bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cyclomatic complexity 17 of func recursiveBFS is high (> 10) (from gocyclo)

…or the same node

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
Calculate shortest paths of children on the way "down" the graph.

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
* removed queueing of nodes in bfs
* returned a value `changed` from shortestPath function to indicate
whether the value returned is a new shortest or not.
* added tests for bfs and updated tests for shortestPath

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
* remove deprecated fields for MinPathStrength
* add field for keeping track of shortest paths distances to a node

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>

for name, tt := range tests {
t.Run(name, func(t *testing.T) {
got := bfs(tt.input)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope tt in function literal (from scopelint)

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
t.Run(name, func(t *testing.T) {
got := bfs(tt.input)

if diff := cmp.Diff(got, tt.want); diff != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the variable on range scope tt in function literal (from scopelint)

Signed-off-by: Colton McCurdy <mccurdyc22@gmail.com>
}
}

func Test_shortestPath(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function Test_shortestPath has 113 lines of code (exceeds 25 allowed). Consider refactoring.

}

// bfs does breadth-first search of the graph, keeping track of the current and past shortest paths.
func bfs(level map[string]*Node) map[string]*Node {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function bfs has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

"github.com/google/go-cmp/cmp"
)

func Test_bfs(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function Test_bfs has 46 lines of code (exceeds 25 allowed). Consider refactoring.

@@ -153,3 +163,36 @@ func TestContainsNode(t *testing.T) {
})
}
}

func TestRoots(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function TestRoots has 27 lines of code (exceeds 25 allowed). Consider refactoring.

}
}

func Test_shortestPath(t *testing.T) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function Test_shortestPath has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

@codeclimate
Copy link

codeclimate bot commented Dec 18, 2019

Code Climate has analyzed commit f1e2a9c and detected 11 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 7
Bug Risk 4

The test coverage on the diff in this pull request is 56.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 66.2%.

View more on Code Climate.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@e510e09). Click here to learn what that means.
The diff coverage is 38.75%.

@@            Coverage Diff            @@
##             master      #16   +/-   ##
=========================================
  Coverage          ?   62.38%           
=========================================
  Files             ?        5           
  Lines             ?      226           
  Branches          ?        0           
=========================================
  Hits              ?      141           
  Misses            ?       73           
  Partials          ?       12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants