Is this a feature envy smell?

Question

In my Application, I wanted to seperate GUI and business logic with some kind of ModelViewPresenter-Design.

Therefore I made some Presenter classes, which have not much more code than the properties my forms are databound to.

To provide persistance (load and save) I made new classes with methods setting the properties of the above mentioned presenter classes to seperate concerns (present data to GUI and persist data).

Since my presenter classes are now only datacontainers without any behavior, I wonder if I should better merge persistance and presenter to simplify my code. It seems very overdone to me as it is at the moment.

Here is some C# snippet from one of the Persistance classes:

public override void LoadData()
{
    Presenter = (PersonalColorSettingsPresenter) PresenterFactory.GetPresenter(m_User, "ColorSettings");
    loadData();
}

private void loadData()
{
    var color = Settings.GetSelectionColor(m_User);

    Presenter.ActiveRowBackColor = color.BackColor;
    Presenter.ActiveRowBackColor2 = color.BackColor2;
    Presenter.IsActiveRowBackgroundGradientActive = color.GradientActive;
    Presenter.ActiveRowForeColor = color.ForeColor;
}

Answer 1

No, you should not merge persistence into your presenter. It may not seem important right now, but doing so would be a violation of the Single Responsibility Principle, and if such decisions were made repeatedly in the long term would lead to code that is difficult to maintain.

As a side note, if you're using the MVP pattern, your persistence layer should interact with your model objects, not with your presenter. The presenter should read data out of the model to pass to the view, and then write the results back to the model after it has finished. A data storage module (e.g.some kind of repository or DAO - please don't use active record here as this violates SRP too) is used to load and save the model objects. The presenter should know as little about how this works as possible, I.e. if possible it shouldn't interact with the data storage at all (have that happen at a higher level) but if it must then have it do so through an abstract interface.