Re: [jira] Commented: (SHINDIG-124) Implementation POJO for opensocial.Enum.xx and opensocial.Message

[Cassie <do...@google.com>]

Taking off the bug because this has turned into a longer thing..

On Tue, Mar 18, 2008 at 7:03 PM, Michael Papp (JIRA) <ji...@apache.org>
wrote:

>
>    [
> https://issues.apache.org/jira/browse/SHINDIG-124?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12579963#action_12579963]
>
> Michael Papp commented on SHINDIG-124:
> --------------------------------------
>
> Hi Cassie - thanks again for your assistance with this code.
>
> 1. Will remove author tag
>
> 2. Here is my reasoning for the implementation:
>    a. Rules for Enum:
>               displayValue can never be null.
>               Key may be null.
>               if displayValue is one of the pre-defined constants, then
> displayValue == Key.toString().


I think this assumption is where things break. We shouldn't assume it will
be key.toString() it should be allowed to be an arbitrary string as well. If
we want to include a helper constructor that just takes in the enum only and
uses enum.toString for the displayValue then that is fine. it only needs to
be a helper though.


>
>               if displayValue is not one of the pre-defined constants,
> then Key = null; displayValue = arbitrary string.
>    b. Enum(Key key, String displayValue) is an unnecessary constructor, as
> either the Key is equivalent to the Value (String), or the Key != Value and
> the Key is null.  Therefore, only the displayValue matters - not the Key.
>  Still, for conformity, I could implement this constructor and throw away
> the Key.


if we go this route, you should really throw away the displayValue... not
the key. then you wouldn't need all of the lookup logic. however, as i
mentioned above, i think both should be preserved in this constructor. so
perhaps this:

public Enum(String displayValue) {
  this(null, displayValue)
}
public Enum(T key) {
  this(key, key.toString())
}
public Enum(T key, String displayValue) {
  // set both
}

>
>    c. The reverse look up on the enum Key properties enforces this
> contract.


Ah, so perhaps I simply conveyed the contract incorrectly. Here is what we
are bound to:
- The displayValue can be any String in the world
- The key must be one of the predefined enums, or null

Thus, the displayValue and the key have no relation to each other (spec wise
that is). Now, I'll give you a case where this makes sense. In Orkut, let's
say they have some radio buttons for your smoker value that look like this:
"unhip, rock it always, when i feel like it"

Which would translate into "no, yes, occasionally" (or whatever). Now, orkut
would probably use their container specific displayValues so that when
gadgets are showing information to the user the user knows what it is
talking about (in relation to the container) Thankfully, the gadgets also
have the keys though which means they know the Orkut's "unhip" is the same
as myspace's "no" without having a bunch of container specific logic in
their app.



>
> As per your previous response and my understanding of the API, this
> implementation fits the requirements (and darn it, I am a stickler for
> requirements).  And while I can see the motivation for providing placeholder
> model classes and let developers fill in the blanks, I generally believe
> that such 'model' implementations should serve as reference implementations
> (go the extra mile, so to speak).  All that said, I really do like your
> generics approach, much cleaner looking, etc.  Let me play around with this
> today and respond with something this afternoon.  Unless you or the team
> think the "contract enforcement" aspect is overboard, I really do urge the
> interested parties to keep this in the implementation.


I completely agree with your contract enforcement approach, I just think
that it doesn't exactly match the contract that the spec wanted to put out
their. Which would be a burden on containers.

So essentially, your coding styles are now aligned with Shindig, but I think
the concept behind it is still a little off. Again, I know this is super
complicated, and it is entirely my fault for not explaining it well enough.

Thank you for all the time you are spending on this!

- Cassie


>
> > Implementation POJO for opensocial.Enum.xx and opensocial.Message
> > -----------------------------------------------------------------
> >
> >                 Key: SHINDIG-124
> >                 URL: https://issues.apache.org/jira/browse/SHINDIG-124
> >             Project: Shindig
> >          Issue Type: New Feature
> >          Components: Gadgets Server - Java
> >            Reporter: Michael Papp
> >         Attachments: fix-124-1-bug.patch.txt, fix-124-2-bug.patch.txt,
> fix-124-bug.patch.txt
> >
> >
> > Submitting proposed implementation for opensocial.Enum and
> opensocial.Enum.Drinker, opensocial.Enum.Gender, and opensocial.Enum.Smoker.
>  This is a best guess pass as there are a number of ways of implementing
> this, and opensocial 0.7 spec for the getKey() is rather nebulous.  Anyway,
> hope this helps.  If approach is right, then I can amend patch to include
> tests.
> > M. Papp
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>