Skip to content
This repository has been archived by the owner on Mar 23, 2021. It is now read-only.

Make NSCoding implementation always encode properties to NSData, and de... #40

Closed

Conversation

marklarr
Copy link
Contributor

encodeWithCoder would use remoteAttributes, which, in some rare scenarios (manually changing properties on an object) would not be accurate.

More importantly, initWithCoder would set the remoteAttributes but not use them to set the properties of the object.

I've included tests, and they should have good coverage!

Open to critique on general coding style or anything else!

@marklarr
Copy link
Contributor Author

@dingbat

id encodedObj = [element remoteDictionaryRepresentationWrapped:NO fromNesting:YES];
id encodedObj = [element dictionaryRepresentationWrapped:NO
fromNesting:YES
remoteOnly:YES];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird... the colons are matching up in xCode, for me. Have you ever heard this happen before?

@@ -607,6 +615,7 @@ - (id) initWithCoder:(NSCoder *)aDecoder
{
self.remoteID = [aDecoder decodeObjectForKey:@"remoteID"];
remoteAttributes = [aDecoder decodeObjectForKey:@"remoteAttributes"];
[self setPropertiesUsingRemoteDictionary:remoteAttributes];
self.remoteDestroyOnNesting = [aDecoder decodeBoolForKey:@"remoteDestroyOnNesting"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My tabbing isn't like this here, either! Very confused..

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Hm. I'm a bit unclear on the reasoning behind this PR. Could you clarify the issue and what exactly this does please? Why the need for a remote and non-remote dictionary representation? And by the way, remoteAttributes is meant to be exactly what was last returned from the server, so it should be able to deviate from the object's specs by design.

Your idea to run [self setPropertiesUsingRemoteDictionary:remoteAttributes] on initWithCoder: is interesting, but I'm not sure this is good since it would risk inconsistent states between coding and encoding. Of course the point of reading a set of saved objects from disk is to reinstantiate them exactly as they were left. Moreover, if you're diligent, you should be overriding encodeWithCoder: in your subclass, saving the state of each of the object's properties, and then restoring them in an overridden initWithCoder:, so calling setPropertiesUsingRemoteDictionary: seems redundant, if not unpredictable.

@marklarr
Copy link
Contributor Author

The thing that caused me to run into this was I was trying to cache these objects as NSData using NSKeyedArchiver and then read them back in later, and all of the properties were set to nil. The setPropertiesUsingRemoteDictionary seemed like a convenient way to get the properties reset once coming through initWithCoder

As for the remoteAttributes being exactly what comes back from the server, that makes sense. I'll need to revise my tests so that this part doesn't matter, since I just allocate a post and manually set the properties (which the coder will then ignore)

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Ah, I see now. In that case, that should certainly be in your subclass's implementation. This kind of thing can't just be the default behavior. (But rather than this sort of hacky-approach, which again, risks not having up-to-date information, I still recommend that you manually encode/decode each one of your subclass's properties, and also make sure you call super when overriding the NSCoding methods.)

Don't think I'll merge this one, thanks though.

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Also, make sure NSKeyedArchiver is right for your use-case. You're probably 1000x better off using CoreData, something I discovered far too late in the game.

@marklarr
Copy link
Contributor Author

Just so we're on the same page, isn't this pulll request causing you to
have have more up to date information? With master the way it is now, if I
archive an object and then unarchive it, everything will be set to nil
except for remote ID. Like you pointed out, isn't the point of NSCoding to
be able to retrieve the object as you left it (this is all only referring
to the initWithCoder method changes)
On Jun 15, 2013 2:43 AM, "dan hassin" notifications@github.com wrote:

Ah, I see now. In that case, that should certainly be in your subclass's
implementation. This kind of thing can't just be the default behavior. (But
rather than this sort of hacky-approach, which again, risks not having
up-to-date information, I still recommend that you manually encode/decode
each one of your subclass's properties, and also make sure you call superwhen overriding the NSCoding methods.)

Don't think I'll merge this one, thanks though.


Reply to this email directly or view it on GitHubhttps://github.com//pull/40#issuecomment-19492797
.

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Yes, that is correct. NSCoding is structured such that each subclass saves each of its own properties that are relevant, and then calls super, going up the hierarchy chain, giving each superclass a chance to encode data. Similarly, it decodes all the properties it encodes. This is done because superclasses have no knowledge of what data their subclasses might have.

So, like I've said above, you need to go in your NSRRemoteObejct subclass and override the following methods:

- (id) initWithCoder:(NSCoder *)aDecoder
{
    if (self = [super initWithCoder:aDecoder]) //note super
    {
        // Restore your properties here
        self.myProperty = [aDecoder decodeBoolForKey:@"myProperty"];
    }
    return self;
}

- (void) encodeWithCoder:(NSCoder *)aCoder
{
    [super encodeWithCoder:aCoder]; //note super

    // Archive all your properties here
    [aCoder encodeObject:self.myProperty forKey:@"myProperty"];
}

@marklarr
Copy link
Contributor Author

I see. I guess, to me, it seems useful to abstract the way properties are
archived so that in each subclass you don't have to manually do it for all
of the the properties. But to each their own :)
On Jun 15, 2013 2:58 AM, "dan hassin" notifications@github.com wrote:

Yes, that is correct. NSCoding is structured such that each subclass saves
each of its own properties that are relevant, and then call super,
going up the hierarchy chain, giving each superclass a chance to encode
data. Similarly, it decodes all the properties it encodes. This is done
because superclasses have no knowledge of what data their subclasses might
have.

So, like I've said above, you need to go in your NSRRemoteObejct subclass
and override the following methods:

  • (id) initWithCoder:(NSCoder *)aDecoder{
    if (self = [super initWithCoder:aDecoder]) //note super
    {
    // Restore your properties here
    self.myProperty = [aDecoder decodeBoolForKey:@"myProperty"];
    }
    return self;}

  • (void) encodeWithCoder:(NSCoder *)aCoder{
    [super encodeWithCoder:aCoder]; //note super

    // Archive all your properties here
    [aCoder encodeObject:self.myProperty forKey:@"myProperty"];}


Reply to this email directly or view it on GitHubhttps://github.com//pull/40#issuecomment-19492908
.

@marklarr marklarr closed this Jun 15, 2013
@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Not just my opinion :)

From the NSCoding protocol:

In keeping with object-oriented design principles, an object being encoded or decoded is responsible for encoding and decoding its instance variables. A coder instructs the object to do so by invoking encodeWithCoder: or initWithCoder:. encodeWithCoder: instructs the object to encode its instance variables to the coder provided; an object can receive this method any number of times. initWithCoder: instructs the object to initialize itself from data in the coder provided; as such, it replaces any other initialization method and is sent only once per object. Any object class that should be codable must adopt the NSCoding protocol and implement its methods.

@marklarr
Copy link
Contributor Author

Sure. They aren't explicitly saying you can't abstract it out, however. Under this logic, each subclass should also archive their remoteID, shouldDestroyNested, and remoteAttributes as well, as you doing it for them is "unpredictable."

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Not at all, how would that be unpredictable?

In keeping with object-oriented design principles, an object being encoded or decoded is responsible for encoding and decoding its instance variables.

Each level of the hierarchy is responsible for the instance variables at its own level. How could NSRRemoteObject possibly know about the properties subclasses below it may have? Those objects may have custom ways of encoding different data types, that you can't just assume your superclass will handle properly. Think about this for not just NSRails, that has mass-property setters, but any hierarchy of objects.

@marklarr
Copy link
Contributor Author

NSRemoteObject already taps into its subclasses properties in many
different ways. That's what makes it an awesome framework and that's why
I'm using it. It looks like neither one of us is budging here, though, so I
can just implement this on my own.
On Jun 15, 2013 3:07 AM, "dan hassin" notifications@github.com wrote:

Not at all, how would that be unpredictable?

In keeping with object-oriented design principles, an object being encoded
or decoded is responsible for encoding and decoding its instance variables.

Each level of the hierarchy is responsible for the instance variables at
its own level. How could NSRRemoteObject possibly know about the properties
subclasses below it may have? Those objects may have custom ways of
encoding different data types, that you can't just assume your superclass
will handle properly. Think about this for not just NSRails, that has
mass-property setters, but any hierarchy of objects.


Reply to this email directly or view it on GitHubhttps://github.com//pull/40#issuecomment-19492998
.

@dingbat
Copy link
Owner

dingbat commented Jun 15, 2013

Cool, no problem. Yeah, NSRails is unique in that way, but I just want to stress that this is how NSCoding is done, and it would be pretty radical to break this really strong foundation-level architecture.

If you have a billion properties and would rather not go through doing this, I understand that. There are other ways, like CoreData, or even extending the Autogen script to generate this code automatically for you might not be a bad idea.

@marklarr
Copy link
Contributor Author

I've been looking into core data. Unfortunately I came into a project late
into the game, and I'm trying to leverage some open source in places where
we used to do things very verbosely. The current way things have been done
is archiving NSData, but I would like to move to core data once I'm
comfortable with how it works
On Jun 15, 2013 3:27 AM, "dan hassin" notifications@github.com wrote:

Cool, no problem. Yeah, NSRails is unique in that way, but I just want to
stress that this is how NSCoding is done, and it would be pretty
radical to break this really strong foundation-level architecture.

If you have a billion properties and would rather not go through doing
this, I understand that. There are other ways, like CoreData, or even
extending the Autogen script to generate this code automatically for you
might not be a bad idea.


Reply to this email directly or view it on GitHubhttps://github.com//pull/40#issuecomment-19493176
.

@marklarr
Copy link
Contributor Author

marklarr commented Mar 4, 2014

Mark Larsen sent you an invitation

Twitter helps you stay connected with what's happening right now and with the people and organizations you care about.

Accept invitation

https://twitter.com/i/61c75b6e-676c-44d0-813b-49aa4a257843

This message was sent by Twitter on behalf of Twitter users who entered your email address to invite you to Twitter.
Unsubscribe: https://twitter.com/i/o?t=1&iid=925bcb081afb4ef690a27b569502107c&uid=0&c=IuVwbCB3dgGh1g1DuA0fH6fIOJ6CQhf8PvhPt%2FTpcKk6YTxV8x5stDTBJSLnAjXbtVzKkFN5ov%2FnPlIlai%2BwUdpvU04ttlyM0k0G5g0psIMljrC9PjGMUg%3D%3D&nid=9+26

Need help?
https://support.twitter.com

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

Successfully merging this pull request may close these issues.

None yet

2 participants