Refactoring: moved code for building JSON message from DS.RESTAdapter to new DS.RESTMessage #535

Closed
wants to merge 4 commits into
from

5 participants

@olmeca

As the REST service I work with (based on Java JSR 311) uses a different JSON message structure than what the DS.RESTAdapter provides, I had to subclass the RESTAdapter. But It turned out I had to override many methods to implement a subtle difference in message structure:

  • createRecord()
  • didCreateRecord()
  • updateRecord()
  • didUpdateRecord()
  • didFindRecord()
  • didFindAll()
  • didFindQuery()

This led to a very fragile subclass: any changes to above methods would have to be reimplemented in my subclass. Hence the reason for moving the concern of serialising/deserializing of the message structure to a separate class, called DS.RESTMessage.
Using this approach I only have to subclass the RESTAdapter to create messages of my RESTMessage subclass. This subclass currently only contains one method. The fragility is eliminated.

Rudi Angela Introduced a separate class DS.RESTMessage to handle the overal struc…
…ture of the JSON messages exchanged with server.
95bb64c
@tchak
Ember.js member

I think you forgot to include the RESTMessage class file in your commit

Rudi Angela Introduced a separate class DS.RESTMessage to handle the overal struc…
…ture of the JSON messages exchanged with server. Now including the new class.
14a07df
@olmeca

You're right. Do I have to make a new pull request?

@tchak
Ember.js member

nop just squash rebase your commit

@tchak
Ember.js member

I am not sure I like the name RESTMessage, maybe RESTDataFormatter or something...

@olmeca

On 'RESTDataFormatter': the 'REST' and 'Formatter' parts are indeed descriptive. But the 'Data' part is a bit too generic to my taste. Perhaps 'RESTMessageFormatter'?
What this class is intended for is handling specific message formats being exchanged with the server. It builds the message in the required format, but it also extracts records from messages received in the format.

@tchak
Ember.js member

I think we do not understand message the same way :)
Anyway, I am not even sure if this is going to be merged. I do understand your rationale, but maybe we could juste add a few hooks on the adapter?

@olmeca

Well, I started out trying to just add a few hooks to the adapter, but ended up using a separate class. The same rationale that led to introducing the Serializer would in my view have to lead to moving the message serialisation out of the adapter. But my main reason for doing it was practical: subclassing the RESTAdapter and reimplementing all the methods that do message formatting leads to a fragile subclass.

@ahawkins

@olmeca What about this:

  1. Use "extract method refactoring" on RESTAdapter to have a method called buildMessageFor(type, record) (or whatever it is)
  2. Refactor all the methods you listed initially to use that buildMessageFor

Also how is this different than serializer?

@stefanpenner
Ember.js member

This idea is great, @olmeca does @twinturbo's suggestion provide enough flexibility? If so that would be my vote.

@olmeca

@twinturbo's suggestion does provide the flexibility, AFAICS. It seemed to me like a better idea to move the details of message building outside of the adapter, in line with the introduction of the serializer. But I accept the current majority stance. I'll add a buildMessage() for building the wrapper JSON and a complementing getMessageContent() for extracting the content out of the wrapper. In some cases the wrapping uses the plural form of the root name, so I'll have to add a parameter indicating if the pluralized form is desired. The new methods will thus be:

  • buildMessage(type, content, plural)
  • getMessageContent(type, message, plural)
Rudi Angela Removed separate DS.RESTMessage for handling the wrapping (and unwrap…
…ping) of records into JSON messages by two new functions in DS.RESTAdapter: buildMessage() and getMessageContent().
3f71330
@igorT
Ember.js member

I think that overriding the serializer rather then the adapter seems like the way to go. JSONSerializer is meant as an abstraction for specifying how to actually serialize to the JSON hash.

@igorT igorT closed this Apr 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment