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

Subclassing Record vs using a Struct #430

Closed
jfahrenkrug opened this issue Oct 12, 2018 · 20 comments
Closed

Subclassing Record vs using a Struct #430

jfahrenkrug opened this issue Oct 12, 2018 · 20 comments
Labels

Comments

@jfahrenkrug
Copy link

What did you do?

I'm new to the library. Thank you! I love it so far, but I'm having trouble deciding whether to make my models be
(a) subclasses of Record or (b) structs that implement Codable, FetchableRecord, MutablePersistableRecord.

The documentation points out that Record has change detection, which is something I'd like. On the other hand it seems as if the struct approach gives me JSON encoding for free, which I also like. When subclassing from Record, it seems as if I need to do the JSON encoding and decoding by hand in the encode(to) and init(row:) methods.

What did you expect to happen?

It would be great if I'd get the same encoding/decoding luxury in Record subclasses. Maybe I'm just doing it wrong :-D.

What happened instead?

It seems as if I get none of the encoding/decoding logic for free that I get when using structs.
There's very little documentation on how to use Record subclasses in advanced scenarios (like having JSON columns), so I might just be doing it wrong.

Environment

GRDB flavor(s): GRDB
GRDB version: 3.4.0
Installation method: CocoaPods
Xcode version: 10.0
Swift version: 4.2
Platform(s) running GRDB: iOS
macOS version running Xcode: High Sierra

Demo Project

(none)

@groue
Copy link
Owner

groue commented Oct 12, 2018

Hello @jfahrenkrug,

You sum up quite well the available options.

The best choice... depends on each situation, and also the taste of the developer.

Back in the early GRDB days, we had no record protocols, but only the Record class. It was born out of my previous Objective-C experience around FMDB, and my pleasure using Ruby's Active Record.

Time has passed, my Swift skills have improved, I learned about the benefits of structs, and Swift brought Codable to us. Eventually my own personal preference has turned to Codable structs + record protocols.

Yet the Record class is still there.

And it will stay. It is important because some people need a record type that is a class (for whatever reason). But the real reason why it is important is that some people expect to subclass a base class, especially people who come from FCModel, Core Data, Realm, Java, Kotlin, etc. I want those people to bring in their personal mental model of what a record type is, and feel welcomed. One doesn't need to learn about protocol-oriented design to use GRDB. And when one eventually learns about Swift structs, Codable, etc, new GRDB options turn available.

I don't really know, currently, how to give a definitive advice. Even if I know quite well that sometimes people just want answers. Do you have your own opinion on this question?

Now let's address some specifics of your question:

The documentation points out that Record has change detection, which is something I'd like.

Record structs can't self-compare, because unlike Record subclasses, they don't have the storage that remembers their previous state. But you can still compare two record structs:

let oldPlayer = try Player.fetchOne(db, key: 1)
var newPlayer = oldPlayer
newPlayer.score = 100
if try newPlayer.updateChanges(db, from: oldPlayer) {
    print("player was modified, and updated in the database")
} else {
    print("player was not modified")
}

On the other hand it seems as if the struct approach gives me JSON encoding for free, which I also like. When subclassing from Record, it seems as if I need to do the JSON encoding and decoding by hand in the encode(to) and init(row:) methods.

Yes. Maybe Record could be enhanced so that it provides a better support for Codable. I don't really know, because my personal focus has turned to record structs: I haven't spent much time on the Record class recently.

@groue groue added the question label Oct 12, 2018
@groue
Copy link
Owner

groue commented Oct 12, 2018

I want those people [who subclass Record] to [...] feel welcomed.

[...] you can subclass the Record class, and get the full toolkit in one go.

There's very little documentation on how to use Record subclasses in advanced scenarios (like having JSON columns)

Maybe Record could be enhanced so that it provides a better support for Codable.

As you can see, there's a palatable tension between the claimed goal, and the reality 😅

Should it be addressed in the documentation, or in the code of the library? That's the question.

@groue
Copy link
Owner

groue commented Oct 12, 2018

As a support for reflexion, and an eventual new paragraph in the Record documentation, here is a quick tour of features and their current availability:

Feature Record subclass Record protocols Codable record
Fetching methods X X X
Persistence methods X X X
SQL generation X X X
Comparison with other records X X X
Self-comparison X
Synthesized table name X X
Synthesized serialization X
Ready-made JSON columns X
Value type (struct) X X
Reference type (class) X X X
// Record subclass
class Player: Record { ... }

// Record protocols
struct Player: FetchableRecord, PersistableRecord { ... }
class Player: FetchableRecord, PersistableRecord { ... }

// Codable record
struct Player: Codable, FetchableRecord, PersistableRecord { ... }
class Player: Codable, FetchableRecord, PersistableRecord { ... }

@jfahrenkrug
Copy link
Author

Hi @groue,

Thank you so much for your quick and thorough reply! It's very helpful to understand the reasons behind the current state of things :)

To answer your question:

Do you have your own opinion on this question?

I personally would like to do things in the most "Swifty" way. I've been an Objective-C guys for years and I'm getting more comfortable with Swift. I think the following line in the README might be a bit misleading:

[...] you can subclass the Record class, and get the full toolkit in one go.

It made me think that I'd get the most features if I subclass Record. So adjusting the documentation would be great. In the long run I think one of two things would need to happen:

  • (A) Drop support for Record and focus on one way of doing things that's well-supported. In other words, be opinionated like Rails :)
  • (B) Support Record as a first-class citizen with all the convenient features that the structs offer.

I tend to think that (A) might be the better approach in the long run. That being said, it would be great to have a code example that shows how to do the JSONSynchronization (https://github.com/groue/GRDB.swift/blob/master/Playgrounds/JSONSynchronization.playground/Contents.swift) with a struct-based approach. This example made me change my structs to subclasses or Record.

Thank you so much for your help and this fantastic framework!

Johannes

@KyleLeneau
Copy link
Contributor

+1 to this issue! I've been similarly confused on some of the direction. 1) I like the Table and it belongs in the docs someplace and 2) I like the push towards more swift standards that embrace the newer patterns away from the likes of Core Data and Realm.

@groue
Copy link
Owner

groue commented Oct 15, 2018

@jfahrenkrug, @KyleLeneau

Thanks for your useful feedback! This discussion is a good way to gather our thoughts and make them clear 👍 And it's totally clear now that the doc needs an update.

The fate of the Record class is not settled yet. Generally speaking, I like it when good practices are made possible. And I prefer opening doors than closing them. Unless I missed something, the Record class is not yet a problem which prevents the library from moving on.

@jfahrenkrug
Copy link
Author

@groue Thank you, you are absolutely right. I seems as if it's primarily a documentation issue. The other half of the issue seems to be that Record is not as convenient to use when it comes to Codable support.

@KyleLeneau
Copy link
Contributor

I agree, this seems like a documentation only type of issue. It might be nice to have a breakdown on it's own about the record types. Don't get me wrong the documentation you have so far is great but it's also a little dense. I might propose a change to organize it a bit more for these types of questions users will have.

@groue
Copy link
Owner

groue commented Oct 16, 2018

I'm all ears. I appreciate a lot critics about documentation because I'm not a native English speaker, not a professional writer, and people usually don't comment on it at all.

I wish there were a fast path inside GRDB records, so that one can quickly setup some working code without thinking much about it. From this starting point, users would then progressively discover the extra information and features that are relevant to their application. Even if the current documentation does not produce this effect, it was my humble goal ;-)

Until now, I used to use the Record class as this entry point. I'm sure Codable records could become a lovely entry point. Maybe they should become this entry point. There are a few barriers on that way:

  • MutablePersistableRecord is a terrifying protocol name. Maybe we could rethink the way we deal about auto-incremented primary keys.
  • class MyType: Record { ... } is easier to the eye than struct MyType: Codable, FetchableRecord, MutablePersistableRecord { ... }. Long lists of protocols are impressive.

😅

@groue
Copy link
Owner

groue commented Oct 16, 2018

class MyType: Record { ... } is easier to the eye than struct MyType: Codable, FetchableRecord, MutablePersistableRecord { ... }. Long lists of protocols are impressive.

OK maybe this is not a real barrier. After all, this is the code I write every day.

MutablePersistableRecord is a terrifying protocol name.

This one is really a problem. It has always been. I could never solve it.

@groue
Copy link
Owner

groue commented Oct 16, 2018

(Live thinking) If we are able to provide the fast path to records with protocols only, then the emphasis on the Record class would naturally vanish. After we get sure it is no longer useful at all, we could eventually sunset this class, stop documenting it, deprecate it, and remove it in a future major release. According to the above table, we would only lose the "self-comparison" feature.

(we don't have to rush, or jump on conclusions)

@jfahrenkrug
Copy link
Author

@groue I really like your suggestions. And above all: Your documentation is outstanding! It is very rare for an open source project to have such great documentation. To improve it, a "cookbook" section could be great too: "I get this kind of nested JSON from my API, how do I best consume, model, persist, and sync it with GRDB?"

About MutablePersistableRecord: If the default case it to use Codable, FetchableRecord, MutablePersistableRecord together, can't we create a Protocol that combines all three and call it something like StructRecord or RecordStruct or something easy like that?

@groue
Copy link
Owner

groue commented Oct 17, 2018

About MutablePersistableRecord: If the default case it to use Codable, FetchableRecord, MutablePersistableRecord together, can't we create a Protocol that combines all three and call it something like StructRecord or RecordStruct or something easy like that?

Those three combinations can easily be found:

  • Decodable + FetchableRecord: read-only records
  • Codable + FetchableRecord + MutablePersistableRecord: r/w records structs with an auto-incremented primary key
  • Codable + FetchableRecord + PersistableRecord: other r/w records

Those three others can be seen, too, when Codable code generation does not really help, or where the performance hit brought by Codable has to be avoided:

  • FetchableRecord
  • FetchableRecord + MutablePersistableRecord
  • FetchableRecord + PersistableRecord

@groue
Copy link
Owner

groue commented Oct 21, 2018

This is a tough documentation challenge. No matter how I try to put it, I get locked in a loop 😅. I need readers to know record features before they can choose which protocols they need. And I need the readers to know the protocols before they know the available features. I'll need several tries until I figure out how to address this quite valid issue.

@jfahrenkrug
Copy link
Author

@groue Thanks so much for your work on this, and no rush :)

groue added a commit that referenced this issue Nov 1, 2018
@groue
Copy link
Owner

groue commented Nov 1, 2018

Folks, I plan to use this very little change in the introduction to record types as a solution for this issue: f7b79e0.

I don't know any straight answer to the question "Should I subclass Record or use a struct?" So I have been deliberately fuzzy and subjective: I hope that a choice between "swifty" and "classic" will match the targeted developer personalities and experiences 🤞😉 I could not add the above table in the doc, because I did not figure out how to make it useful.

@groue groue closed this as completed Nov 1, 2018
@jfahrenkrug
Copy link
Author

@groue Very nice! I think to address my original concern, it would be fantastic to mention in the docs that certain features are only available with the struct approach, like JSON columns etc. But it's up to you whether you want to add that. Thanks again!

@groue
Copy link
Owner

groue commented Nov 1, 2018

It's not only up to me :-) If you or another user feels like writing and helping other users make the best of record types, I and other GRDB collaborators will be glad reviewing a pull request. It's not only about code, as you see!

@gennaios
Copy link

gennaios commented May 5, 2019

Hello, if you have the time, I’m very curious about your opinion of this, particularly what’s said at the end.

https://medium.com/commencis/stop-using-structs-e1be9a86376f

I’m not sure if there’s something particular about the Record class that makes it less performant or if it’s that it uses a class over struct. My use is mainly a FetchedRecordsController for a table view. Currently not that large, less than 20,000 records, though that is perhaps a fair amount. Performance is already very fast though if there is some potential benefit to classes as suggested by that article — I am not so advanced in Swift to be able to analyze —, maybe it is worth switching.

@groue
Copy link
Owner

groue commented May 5, 2019

@gennaios GRDB is value/reference type agnostic. You can define records from structs, classes, or subclasses of Record.

// OK
struct Player: FetchableRecord, PersistableRecord { ... }

// Sure
class Player: FetchableRecord, PersistableRecord { ... }

// Yeah
class Player: Record { ... }

This means that the choice is eventually up to the application developers.

Now I personally favor structs because I'm sure they won't mutate under my feet, and I can pass them across threads without thinking too much. This is so comforting.

But this can not be an absolute advice: only you can take a decision about the lifetime of your records, instrument your app, perform experiments, and make a wise choice.

You can also check the Performance page, if you want, even its goal is to compare the inner guts of several database libraries, and NOT to provide accurate numbers about realistic database uses.

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

No branches or pull requests

4 participants