Edge has no __toString()

[clemens-tolboom]

Trying to

echo $edge;

a single edge is not an option right now.

Should we add this?

[clue]

Depends on:

• What's the use case?
• What's the expected output?

[clemens-tolboom]

I expected a list of vertex IDs and a type. When developing with Graph it is handy not to single step but just print an item. I could have done

echo $edge->getGraph();

but that would be information overload. IMHO every class should have a simple __toString() to give some info when needed.

[clue]

What about edge attributes such as flow, capacity, possibly even graphviz attributes? Perhaps people will even sub-class the Edge class? Imo, once we start adding a __toString() method, we should make sure it's actually complete..

I can see were you're coming from, but I'm not quite sure how we should address these concerns?

[clue]

After some more consideration, how about this:

class Edge
{
   // [...]
    public function __toString()
    {
        return $this->getGraph()->getExporter()->exportEdge($this);
    }
}

This way, the Edge (and in the future also possibly the Vertex or eventually even a Subgraph) do not have to be cluttered with outputting their attributes. We can simply extend the ExporterInterface instead and thus allow for a cleaner separation of concerns. Also, the exporter can be easily exchanged during runtime (Graph::setExporter()).

The default output could perhaps look something like this:

$edge = $vA->createEdge($vB);
echo $edge; // A -- B

$edge = $vA->createEdgeTo($vB);
echo $edge; // A -> B

$edge = $vA->createEdgeTo($vB)->setFlow(10)->setCapacity(20)->setWeight(30);
echo $edge; // A -[10/20/30]-> B

[clemens-tolboom]

Proposed solution sound great indeed. The exporter decides what to dump. 👍

[clue]

I've tinkered around with the idea of extending our ExporterInterface to also dump single Edge instances, but in the end we'd end up with additional interfaces for exporting just edges, an interface for dumping just vertices, additional decorator interfaces, etc.

Simply put, I'm not quite happy with this concept, so perhaps we should reconsider if adding a __toString() is actually a good idea. See also __toString() or not __toString()?.

An alternative, and more explicit, approach would perhaps be something along the lines of:

$exporter = new HumanReadableEdgeExporter();

echo $exporter->exportEdge($e1);
echo $exporter->exportEdge($e2);

[clemens-tolboom]

Ain't this fixed already?

[clue]

Depends on what your answer to my above question is :)

Simply put, I'm not quite happy with this concept, so perhaps we should reconsider if adding a __toString() is actually a good idea. See also __toString() or not __toString()?.

[clemens-tolboom]

doh ... rereading I completely missed your point :/ ... __toString() is less flexible then having exporters.

Do we really need edge exporters or is that just an exporter ignoring graph and vertices? If the latter we can configure the exporter only to 'print' graph, edge or vertices depending on the current needs.

I'm indifferent with the outcome as long as echo $graph gives user readable data.

[clue]

Do we really need edge exporters […] ?

IMHO, yes. See the linked article for some arguments on how it breaks the SOLID principle. Also, once #26 lands and people are encouraged to extend the base classes, we won't be able to handle exporting their parameters either.

Even currently, it's highly ambiguous what exactly to "export" (see my second comment).

I can certainly see a desire to "export an edge", but I'm not convinced on that we can actually provide a satisfying result in the first place. Providing some base edge exporters would be a good start for others to build upon, IMHO.

[clue]

Simply put, I'm not quite happy with this concept, so perhaps we should reconsider if adding a __toString() is actually a good idea. See also __toString() or not __toString()?.

I still think it's a bad idea to implement __toString() on either the Graph, Vertex or Edge.

We're working towards splitting off individual components via #120.
As such, GraphViz and GraphML have been moved to individual components already.
The TrivialGraphExporter is the only remaining exporter in this repo and will eventually be split off via #121.
There's little purpose in keeping the then useless Exporter namespace. It will be removed via #122.

As such, I don't think this repo should provide a default exporter for either of our entities.

That being said, I still consider external Exporters a valid and useful feature. They provide a SOLID design and work out well.