Need help eliminating code smell (SWITCH)

[Frank O'Hara]

Hi there, I'm having difficulty seeing how to eliminate this
particular smell, well at least I suspect it as being smelly given
that the switch is being repeated and I can see it repeating
elsewhere.

I have code in my DAL that returns and executes an action based on a
type (i.e. it returns the proper datamapper to exectue the action
against the database). I can't seem to think of the manner in which
to eliminate my switch statement(s) polymorphically. See code below:

class DemoDataProvider
{
private bool _isInTransaction;
private List<ScheduledAction> _actions = new
List<ScheduledAction>();

public bool IsInTransaction
{
get { return _isInTransaction; }
private set { _isInTransaction = value; }
}

public void Create(object item)
{
ScheduledAction action = createNewAction(item,
ScheduledAction.ActionType.Create);

manageAction(action);
}
public void Delete(object item)
{
ScheduledAction action = createNewAction(item,
ScheduledAction.ActionType.Delete);

manageAction(action);
}
public IList<T> GetAll<T>() where T : class, new()
{
return getDataMapper<T>().GetAll();
}

private ScheduledAction createNewAction(object item,
ScheduledAction.ActionType actionType)
{
if (item == null)
{
throw new ArgumentNullException();
}
ScheduledAction action = new ScheduledAction(item,
actionType);
return action;
}
private void executeAction(ScheduledAction action)
{
if (action.Target.GetType() == typeof(FormsBase) ||
action.Target.GetType().BaseType == typeof(FormsBase))
{
this.executeScheduledAction<FormsBase>(action);
}
else
{
throw new Exception("Unknown entity type");
}
}
private void executeScheduledAction<T>(ScheduledAction action)
{
T target = (T)action.Target;

IDataMapper<T> dataMapper = (IDataMapper<T>)
getDataMapper<T>();

switch (action.Type)
{
case ScheduledAction.ActionType.Create:
dataMapper.Create(target);
break;
case ScheduledAction.ActionType.Delete:
dataMapper.Delete(target);
break;
case ScheduledAction.ActionType.Update:
dataMapper.Update(target);
break;
}
}

private IDataMapper<T> getDataMapper<T>()
{
if (typeof(T) == typeof(FormsBase) ||
typeof(T).BaseType == typeof(FormsBase) ||
typeof(T) == typeof(Form1))
{
return (IDataMapper<T>)new FormDataMapper();
}
else
{
throw new Exception("Unknown entity type");
}
}
private void manageAction(ScheduledAction action)
{
if (this.IsInTransaction)
{
_actions.Add(action);
}
else
{
executeAction(action);
}
}
}

So obviously the switches in 'executeAction' and 'getDataMapper' are
going to get rather large once I add more domain objects and they are
also going to be virtually duplicated. The generics I'm using is also
making things more challenging (in terms of my ability to see the
solution).

I'm still just learning the finer details about writing cleaner code
and I'm trying to make a very conscious effort to ensure that my code
going forward is much better.

Any help would be appreciated.

Thanks,
Frank

[Paul Sinnett]

Will executeAction and getDataMapper always handle the same cases?
They are already out of sync: getDataMapper handles the case of type
Form1 but executeAction doesn't.

I don't know what's allowed with Java generics. Can you do something
like? :

private void executeAction(ScheduledAction action)
{
if (action.Target.GetType() == typeof(FormsBase) ||
action.Target.GetType().BaseType == typeof(FormsBase))
{

IDataMapper<FormsBase> mapper = (IDataMapper<FormsBase>)new
FormDataMapper();
this.executeScheduledAction<FormsBase>(action, mapper);

}
else
{
throw new Exception("Unknown entity type");
}
}

private void executeScheduledAction<T>(ScheduledAction action,
IDataMapper<T> dataMapper)
{
T target = (T)action.Target;

switch (action.Type)
{
...
}
}

[Frank O'Hara]

Paul, thanks for the response. This is actually written in C# but it
doesn't really matter I guess.

I had already tried your suggestion, if for no other reason, to get
rid of the getDataMapper function but it is called from another
another method and not necessarily coupled to executeScheduledAction.

Say for the sake of arguement that executeAction and getDataMapper
will always handle the same classes, are you suggesting to include
those functions in each of the classes that are part of my
conditional? If that is what you're getting at, then I'm going to
have to write code in all the classes (or a base class of some sort)
that contains logic that should *really* be in the data provider class
(i.e. transaction logic). My preference would be to NOT put that
logic in a base class that business entities share.

Thoughts?
Frank

> }- Hide quoted text -
>
> - Show quoted text -

[Paul Sinnett]

On May 28, 6:48 pm, "Frank O'Hara" <mrpubni...@gmail.com> wrote:
> Say for the sake of arguement that executeAction and getDataMapper
> will always handle the same classes, are you suggesting to include
> those functions in each of the classes that are part of my
> conditional?  If that is what you're getting at, then I'm going to
> have to write code in all the classes (or a base class of some sort)
> that contains logic that should *really* be in the data provider class
> (i.e. transaction logic).  My preference would be to NOT put that
> logic in a base class that business entities share.
>
> Thoughts?

Sorry for the delay. Google failed to prompt me about your reply until
today :(

Is the idea that you want to extend like this:

private IDataMapper<T> getDataMapper<T>()
{
if (typeof(T) == typeof(FormsBase) ||
typeof(T).BaseType == typeof(FormsBase) ||
typeof(T) == typeof(Form1))
{
return (IDataMapper<T>)new FormDataMapper();
}

else if (typeof(T) == typeof(OtherBase))
{
return (IDataMapper<T>)new OtherDataMapper();
}
else if (typeof(T) == ...)
{
return (IDataMapper<T>)new ...

}
else
{
throw new Exception("Unknown entity type");
}
}

Would that even work? Wouldn't the compiler complain if you called for
example:

getDataMapper<OtherBase>()

because of the cast from FormDataMapper() to IDataMapper<OtherBase>
and so on?

I'll get hold of mono and try some things out.