On 11 April 2014 18:54, Adam Cox <notifications@github.com> wrote:
>
>  I thought a bit about the organization of these classes -- and trying to
> stay DRY. The polymorphic behavior seems clear to me (but probably because
> I wrote it!). If source/target methods were eliminated from the base class,
> then the base class's -dictionary... method wouldn't properly work and we
> should eliminate that as well. But I think that this means when we add all
> of the other replication parameters later, we'll have to code that up
> separately for both push and pull. And it quickly starts to feel like we
> should just eliminate the base class.
>
> Also, I am trying to consider that people may want to use these classes
> elsewhere, perhaps if they want to save the replication dictionary into a
> separate database and they want to write generic code to handle a
> CDTReplication object. It seems to me that if we don't include the -source:
> -target: and -(id) filter: methods in the base class, it can't be used
> generically
>
> Also, I initially went the route of parsing out all of the components
> (username, password, scheme, port, baseurl, remotedbname), but felt like
> that was just way too much code for something as simple as a user wanting
> to set a URL, which we can instruct them via documentation on how to set.
> If we break those out into components, then we have a write a method that
> constructs the NSURL each time rather than allowing the user just setting a
> URL. And there would need to be a number of checks in the code. Asking for
> a NSURL seemed a lot simpler to me. Obviously I can do it and still support
> a method to let a user call something like -setRemoteURL:(NSURL*), but I
> had my reasons not to. It's too bad that NSURL doesn't have - setUser and
> -setPassword methods.
>
I'd say, perhaps, then: remoteDatabase, username, password as separate
properties.

> And the -(id) filter method is also in the base class to support generic
> behavior. But, I do see your point and I could do a [replication
> respondsToSelector:@selector <https://github.com/selector>(filter)]
> inside CDTReplicationFactory. However, they both WILL get filter support
> and having it in the base class would eliminate this need to check in every
> piece of code that wants to use it.
>
Looks like this was addressed in another commit/comment.

>  I do agree that the Push/Pull classes are not as self describing as they
> could be. One thing would be to redeclare the methods in header files (that
> doesn't mess up an object's access to it's super method, does it?).
>
> I actually didn't like the Java code (sorry if you wrote it :) ) because
> it doesn't seem to really use the base class and the code repeats itself.
> Maybe that's okay, however, for these classes since we expect them to be
> very static over the years. By removing the type from the source/target
> methods and putting them in the remote/localDatastore properties, the base
> class becomes more flexible.
>
> Hrm... but maybe flexibility isn't what we need and I was trying to be too
> fancy....
>
For code like this, I'd go for simple over flexible -- I don't think many
people will be reusing it. The classes in this case are simple enough that
I think the abstraction makes them harder rather than easier to understand
-- the parent class wasn't encapsulating the bahaviour and the complexity
was leaking into the child classes.

You'll see a pattern with my comments for simplicity :)

>  thanks again for discussing this with me
>
A pleasure.

I thought this might be a good way to simplify the construction of
replications:

http://www.annema.me/blog/post/2014/4/4/the-builder-pattern-in-objective-c

But don't take this as a directive :D