gonum / graph

Graph packages for the Go language [DEPRECATED]
250 stars 39 forks source link

internal: Add Reset method to NodeStack #187

Closed tcharding closed 7 years ago

tcharding commented 7 years ago

When doing breadth first traversal the NodeStack is reset by doing a direct slice re-size on the underlying data structure (slice) used to implement the stack. This violates the stack abstraction provided by NodeStack. Instead of directly re-sizing the slice we can add a method to NodeStack to reset the underlying data structure.

This patch adds Reset() method to NodeStack. This allows the stack to be reset while maintaining data abstraction.

kortschak commented 7 years ago

The package in internal. The only reason a Reset method is provided by for the queue is that it requires two operations, a stack reset is trivial, so I don't think this is necessary.

tcharding commented 7 years ago

Thanks for your reply. On a language specific topic, does the type declaration of NodeStack (type NodeStack []graph.Node) also imply that the slice is accessible to client code. i.e if it were meant to be an abstraction it would have been wrapped in a struct. Either way, closing PR. Thanks again.

kortschak commented 7 years ago

What you see is the interaction between the language and the standard build chain. The language defines NodeStack as exported because it has an uppercase initial rune. The build chain will not make it available to packages outside gonum/graph because it is in an internal path. It's not really meant as an abstraction in the OO sense. It's a convenience (so yes, an abstraction). One thing to recognise here is that religious observance of programming concepts is not so much valued here. Pragmatism is more highly valued.

tcharding commented 7 years ago

Thanks for the response Dan.