[MonoDevelop] Some changes to ambiences

[Michael Hutchinson]

On Fri, Nov 28, 2008 at 2:51 PM, Mike Krüger <mkrueger at novell.com> wrote:
> Hi
>
>> How's at any harder? There's a single override to implement, and they
>> know that the argument is an OutputSettings object so they don't have
>> to do type checks. The OutputSettings object could even have
>> convenience properties for accessing the flags, e.g.
>> bool UseMarkup { get { return flags & OutputFlags.UseMarkup != 0; } }
>> The outputsettings object could handle the null checks on the
>> postprocessing delegates too, e.g.
>>
>
> I've thousands of methods that are:
>
> VisitSomething (Something something, object data)
> {
> ..
>        GetString (something.SomeObject, data)
> ..
>        IsValidSomething (something, data)
> }
>
> etc. The visitors are operating on objects. This method prevents that
> I've to upcast the data objects in each visit method as ambience
> implementor.

But an OutputSettings contains the flags, so I don't understand why
you can't require that the data object is an OutputSettings, and have
overloads on the *public* Ambience API (i.e. GetString etc) that will
accept an OutputFlags or an OutputSettings.

The main problem, though, is API usability. Imagine that, as a new
user of this API, you see the following signature in code completion:
public string GetString (IDomVisitable domVisitable, object settings)
You'd have no clear idea what the settings object is meant to be.

It does not make sense for the public ambience API to reflect the
limitations of the visitor pattern if we can avoid it.

As a bonus, all of the static protected helper methods on Ambience can
be moved to instance members of the OutputSettings object, e.g.
protected static bool IncludeModifiers (object settings)
becomes
public bool IncludeModifiers
on the settings object, and extracts the value from the flags enum in
the settings. IMO,
if (settings.IncludeModifiers) {}
is more readable than
if (IncludeModifiers (settings)) {}

-- 
Michael Hutchinson
http://mjhutchinson.com