Members with type mapping should handle nulls

[herskinduk]

If a method or property on a wrapper needs to convert the return type - it currently does not check for null values when new'ing up wrappers. The consumer would have to look at InnerObject to see verify nulls.

Two approaches:

1. The wrapped method/property only converts if target is not null. Else it will return null. (hard)
2. The wrapped method/property does what it does today and IAutoWrapped exposes a HasValue method. (easy)

Let's see a show of hands.

[cardinal252]

I think both are ok - if the latter ensure to implement INullable if possible.

[kevinobee]

Or use a null object pattern and use an is test in the caller

-----Original Message-----
From: "Kern Herskind Nightingale" notifications@github.com
Sent: ‎21/‎09/‎2014 15:46
To: "herskinduk/AutoWrapping" AutoWrapping@noreply.github.com
Subject: [AutoWrapping] Members with type mapping should handle nulls (#18)

If a method or property on a wrapper needs to convert the return type - it currently does not check for null values when new'ing up wrappers. The consumer would have to look at InnerObject to see verify nulls.
Two approaches:

1. The wrapped method/property only converts if target is not null. Else it will return null. (hard)
2. The wrapped method/property does what it does today and IAutoWrapped exposes a HasValue method. (easy)
   Let's see a show of hands.
   —
   Reply to this email directly or view it on GitHub.=

[Nav-Marwaha]

I like Kevin's idea, but that would be roughly the same amount of effort as 1) though would it not?

[herskinduk]

Number 1 requires all passthroughs to do:

_innerObject == null ? null : new TypeWrapper(_innerObject)

Number 2 is a single IsNull/HasValue method on a BaseInstanceWrapper. (@cardinal252 I would probably not use INullable. It lives in the SQL assemblies? no?)

Number 3 (@kevinobee's)... I like aspects of the idea - but I have reservations around introducing more cogs in the member forwarding mechanics (the type mapping is painfull enough - like following the 🐰). Is there a low impact way of doing it?

[cardinal252]

Sorry I meant Nullable half awake !!!
On 22 Sep 2014 13:17, "Kern Herskind Nightingale" notifications@github.com
wrote:

Number 1 requires all passthroughs to do:

_innerObject == null ? null : new TypeWrapper(_innerObject)

Number 2 is a single IsNull/HasValue method on a BaseInstanceWrapper. (
@cardinal252 https://github.com/cardinal252 I would probably not use
INullable. It lives in the SQL assemblies? no?)

Number 3 (@kevinobee https://github.com/kevinobee's)... I like aspects
of the idea - but I have reservations around introducing more cogs in the
member forwarding mechanics (the type mapping is painfull enough - like
following the [image: 🐰]). Is there a low impact way of doing it?

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

[kevinobee]

null is never an appropriate value because it leads to
NullReferenceExceptions. The Null Object
http://en.wikipedia.org/wiki/Null_Object_pattern pattern provides a
better alternative to deal with situations where a value might be undefined.

-- @ploeh

I rest my case :)

[herskinduk]

@kevinobee please don't rest you case!

This it will change the behaviour of the API we are wrapping. I cannot work out how easy it would be to implement NullObjectWrapper...

[cardinal252]

Much of the existing logic and use cases for systems will probably already have null checking in them - being able to check for null on the wrapping layer might satisfy retro-fit use cases better ?

Also, I also share Kern's concern that it adds to the forwarding mechanics as the aim of this solution is to provide a low maintenance abstraction layer, you would be (by adding null objects) changing the original behaviour - this could potentially lead to a) testing your abstraction layer (against design goal) and b) increased work if / when you would choose to remove the abstraction layer at a later date (say it becomes unsupported or they upgrade to a version of the api that provides its own)

[herskinduk]

@kevinobee suggests Maybe:
http://lostechies.com/derickbailey/2010/09/30/monads-in-c-which-part-is-the-monad/

[herskinduk]

Ok ok ok... I have been considering the impact of this and I see merit in most of the options voiced.

May I suggest that we apply my favourite design principle: SEP

The types we are worrying about are the ones covered by type translation. Type translation is is configurable. So AutoWrapping should not worry to much about which return types needs a null check or a monad. We can leave that up to the consumer.

Would it be worth doing:

• Add HasValue to the IAutoWrappedInstance
• Implement a Maybe<T> where T is IAutoWrappedInstance (it'll need to lookout for HasValue rather than null.
• Add examples of the possible type translations (currently only straight through is shown)

[herskinduk]

@kevinobee On the subject of Maybe, I understand this one better than Derick's:
https://github.com/ploeh/Booking/blob/master/BookingDomainModel/Maybe.cs

[herskinduk]

Here is an example of three mappings (maybe, null-check, dumb):

new TypeTranslationInfo() { 
    ActualType = typeof(Sitecore.Data.Items.Item), 
    TranslatedType = "AutoWrapping.Maybe<Sitecore.Data.Items.IItem>", 
    ForwardTranslationExpression = "new AutoWrapping.Maybe<Sitecore.Data.Items.IItem>(new Sitecore.Data.Items.ItemWrapper(target))",
    ReverseTranslationExpression = "target.DefaultIfEmpty(null).SingleOrDefault().InnerWrappedObject as Sitecore.Data.Items.Item"
},
new TypeTranslationInfo() { 
    ActualType = typeof(Sitecore.Data.Database), 
    TranslatedType = "Sitecore.Data.IDatabase", 
    ForwardTranslationExpression = "(target==null) ? null : new Sitecore.Data.DatabaseWrapper(target)",
    ReverseTranslationExpression = "(target==null) ? null : target.InnerWrappedObject as Sitecore.Data.Database"
},
new TypeTranslationInfo() { 
    ActualType = typeof(Sitecore.Data.Fields.Field), 
    TranslatedType = "Sitecore.Data.Fields.IField", 
    ForwardTranslationExpression = "new Sitecore.Data.Fields.FieldWrapper(target)",
    ReverseTranslationExpression = "target.InnerWrappedObject as Sitecore.Data.Fields.Field"
},

[cardinal252]

My personal view on this is that I prefer Maybe, but I think given the
aim of this is a much wider audience and much of that audience will likely
not be strong programmers and / or fundamentally lazy (so may not want to
go look up maybe), I think that the second null-check option is going to
cause the least confusion and still achieve the fundamental aim. Old
principle K.I.S.S.

On 24 September 2014 22:44, Kern Herskind Nightingale <
notifications@github.com> wrote:

Here is an example of three mappings (maybe, null-check, dumb):

new TypeTranslationInfo() {
ActualType = typeof(Sitecore.Data.Items.Item),
TranslatedType = "AutoWrapping.Maybe<Sitecore.Data.Items.IItem>",
ForwardTranslationExpression = "new AutoWrapping.Maybe<Sitecore.Data.Items.IItem>(new Sitecore.Data.Items.ItemWrapper(target))",
ReverseTranslationExpression = "target.DefaultIfEmpty(null).SingleOrDefault().InnerWrappedObject as Sitecore.Data.Items.Item"
},
new TypeTranslationInfo() {
ActualType = typeof(Sitecore.Data.Database),
TranslatedType = "Sitecore.Data.IDatabase",
ForwardTranslationExpression = "(target==null) ? null : new Sitecore.Data.DatabaseWrapper(target)",
ReverseTranslationExpression = "(target==null) ? null : target.InnerWrappedObject as Sitecore.Data.Database"
},
new TypeTranslationInfo() {
ActualType = typeof(Sitecore.Data.Fields.Field),
TranslatedType = "Sitecore.Data.Fields.IField",
ForwardTranslationExpression = "new Sitecore.Data.Fields.FieldWrapper(target)",
ReverseTranslationExpression = "target.InnerWrappedObject as Sitecore.Data.Fields.Field"
},

—
Reply to this email directly or view it on GitHub
#18 (comment)
.

Nathanael Mann
Web Developer
Telephone: 07885 970777
Email nathanael.mann@cit-design.co.uk