davecom / SwiftGraph

A Graph Data Structure in Pure Swift
Apache License 2.0
757 stars 80 forks source link

Make graph classes structs? #45

Closed ferranpujolcamins closed 6 years ago

ferranpujolcamins commented 6 years ago

Since now Graph is a protocol, we don't use inheritance that much. The only class that is still inheriting is UniqueElementsGraph. Shall we convert all the graph classes to structs? UnweightedGraph and WeightedGraph store the graph in a couple of arrays, which are value types. So it makes sense for the graph classes to also be value types from this point of view. This will also allow us to remove the Codable specific classes and implement it in an extension right?

davecom commented 6 years ago

We've looked into this in the past. There are a couple reasons we decided not to do it:

So, for this decrease in flexibility, what do we gain? You're right, we will get to put Codable support right into the structs through a conditionally conforming extension, but I think this will be possible with classes in future versions of Swift too. As for UniqueElementsGraph wouldn't being a struct actually involve more code then, since this could no longer be a subclass and we would have to reimplement much of UnweightedGraph in UniqueElementsGraph?

I know it's trendy in Swift to use structs as much as possible, but it's not clear to me the win here in particular. I'm open to further discussing this topic if we can find some big wins, but I think the losses may outweigh the wins. To quote Chris Lattner:

What irritates me is when people say classes are bad. Or subclassing is bad. Thatʼs totally false. Classes are super important. Reference semantics are super important. If anything, the thing thatʼs wrong is to say, one thing is bad and the other thing is good. These are all different tools in our toolbox, and theyʼre used to solve different kinds of problems. And the bad thing is to say, classes are awesome and should be used to solve all problems, or structs are awesome and should be used to solve all problems. I think a lot of people that say subclassing is bad are reacting to the “overclassification” of Objective-C.

Source: https://oleb.net/blog/2017/06/chris-lattner-wwdc-swift-panel/

ferranpujolcamins commented 6 years ago

Thanks for the explanation!

Lose the ability to share the memory of a graph between different view controllers/other control classes in an app using the same graph; we actually want reference semantics often instead of value semantics when sharing a large graph between different components of an app

I see this the other way around: if Un/WeightedGraph is a value type, users can easily wrap it in a thing class and get reference semantics. But getting value semantics from a class is more involved.

Lose the ability of users of the library to utilize inheritance; it's very convenient to be able to subclass UnweightedGraph and WeightedGraph and add your own functionality on top

The code reuse you can achieve with inheritance can also be achieved through composition. I acknowledge sometimes you need more boilerplate though.

Following the idea of a value type wrapped by a reference type: we could provide a class wrapping the value type and with the composition boilerplate already done. This class would be ready to be sub-classed and it's methods overridden.

I think this idea is worth exploring. Of course there's no need to rush. Let's keep the discussion open?

davecom commented 6 years ago

Sure, we can keep the discussion open. Again, I'm open to it if there are wins. But the workaround you are suggesting to "regain" the reference semantics that we already have (wrapping the structs in a class) seems counterintuitive to this being a good idea. I would ask again, what specifically are we gaining with structs that we don't already have with classes? I think the ability to do some conditional conformance immediately to eliminate the Codable subclasses maybe one thing, but I don't think that offsets the losses in simplicity.

ferranpujolcamins commented 6 years ago

After a quick prototype. I have to admit that it's too much boilerplate. I didn't realize how many methods we would need to write just to call them on the value type (the search extension for example introduces a lot of methods). Thank you for discussing it with me.