Hacker News Reader

Comment by beerkg | Hacker News Reader

I totally agree with your points, but this approach has one big advantage - it's dead simple - define attributes and mapping and you're good to go.

View on HN

You don't have to sacrifice that simplicity, actually. (And I insist on that simplicity being a wrong type, it'll bite users of your library basically right away, when they try to use it for anything apart from storage/serialisation)

But you can just give an upgrade path! consider something like this:

  class Address
    attr_accessor :street, :city
  end

  class Person
    attr_accessor :address
  end

  class AddressMapper < Shale::Mapper
    mapped_class Address
    attribute :street, Shale::Type::String
    attribute :city, Shale::Type::String
  end

  class PersonMapper < Shale::Mapper
    mapped_class Person
    attribute :address, AddressMapper
  end

  # use like this
  PersonMapper.from_xml("...."); PersonMapper.to_xml(person)

and then, for _dead_ simplicity, you can add another method generate_mapped_class "Person"

which will define that PORO class for user for extra DRYness. API is basically the same, no repetition, but amount of rewrite with new requirements is drastically less.

I'm not asking you to rewrite your library, and I probably won't write and release mine, just saying that considering future self isn't that hard. And yeah, it's a bit of a rant about ActiveRecord from user of Rails, since 2006.

I haven’t looked at the Shale source code, but I suspect that it would not be hard to add `mapped_class` support the way you’ve described it, so that the business objects are not themselves mapper instances. At a guess, the `from_xml` probably does something like (vastly over simplified):

    def from_xml(xml_string)
      new.tap { |o|
        parse_xml(xml_string) do |key, value|
          o.__send__(:"#{key}=", value)
        end
      }
    end

It would then be possible to change this to:

    def from_xml(xml_string)
      (mapped_class || self).new.tap { |o|
        parse_xml(xml_string) do |key, value|
          o.__send__(:"#{key}=", value)
        end
      }
    end

This would make it easier to solve a larger problem of needing to serialize the same business object in different ways for different consumers with different levels of detail. It would also permit the construction of mappers for temporary objects that contain the details for more complex serializations that have indirect connections.

I like it actually, using POROs (or any class for that matter) is definitely a big advantage. Maybe I implement something like that for version 2 :)

An added advantage of this approach is that it allows clean integration w/any other mapper or library. E.g. you could define a mapping to your Sequel or ActiveRecord models, and in one go you have the ability to roundtrip between JSON/XML etc. and the ORM.

To the point of rewriting: A halfway point is to drop inheritance in favour of include/extend'ing the models. If that's done cautiously, it allows for co-existing with model objects from libraries that require inheritance. That is, this:

   class Person
     include Shale::Mapper
   end

is preferable to

  class Person < Shale::Mapper
  end

Agreed that this is a big advantage. I’ve switched to having a separate set of serialization objects with straightforward copy constructors or mapping functions and let the serialization library do the job against those. I used to hand roll the serialization, but this is admittedly user.

Reply on news.ycombinator.com