4
\$\begingroup\$

I'm wondering if this code can be simplified. I'm less than thrilled about the fact that I repeat the Draw method in both CatModel and DogModel. For that matter, I'm not real happy that that method is even in the models at all. Trying to move it up to the AnimalModel superclass forces a cast.

EDIT: The "Model" classes should probably have been named "ViewModel" as that's the role they are playing in the actual project.

namespace GenericInterface
{
  public interface IDrawer {
  }

  public interface IDrawer<TModel>: IDrawer
    where TModel: IAnimalModel<TModel> 
  {
    void Draw(TModel model);
  }

  public interface IAnimalModel 
  {
    void Draw();
  }

  public interface IAnimalModel<TModel>: IAnimalModel
    where TModel: IAnimalModel<TModel> 
  {
    IDrawer<TModel> GetDrawer();  // parent interface might like to know that it can get some kind of drawer, but it gets messy with the types.
  }

  public class CatDrawer: IDrawer<CatModel> {
    // eventually, we want to have different types of CatDrawers.
    public void Draw(CatModel model) {
      Console.WriteLine(model.NWhiskers + " whiskers");
    }
  }

  public abstract class AnimalModel {
    public abstract void Draw();
  }

  public abstract class AnimalModel<TModel>: AnimalModel, IAnimalModel<TModel> 
    where TModel: class, IAnimalModel<TModel> {
    public abstract IDrawer<TModel> GetDrawer();
  }

  public class CatModel: AnimalModel<CatModel> {
    // The repetition of "CatModel" in the above line is less than ideal. It is driven by the fact that, as best I can tell, there is no way to say that a type parameter should always be equal to the name of the current class.
    public int NWhiskers { get; set; }
    // <summary>Would eventually become IDrawer<CatModel> GetDrawer(DrawerFactory factory) { return factory.GetCatDrawer(this); }
    public override IDrawer<CatModel> GetDrawer() {
      return new CatDrawer();
    }
    // Once we have different CatDrawers, this would become 
    // public override void Draw(DrawerFactory factory) { this.GetDrawer(factory).Draw(this); {
    public override void Draw() {
      this.GetDrawer().Draw(this);
    }
  }

  public class DogModel: AnimalModel<DogModel> 
  {
    public string Breed { get; set; }
    public override IDrawer<DogModel> GetDrawer() {
      return new DogDrawer();
    }
    public override void Draw() {
      this.GetDrawer().Draw(this);
    }
  }

  public class DogDrawer: IDrawer<DogModel> {
    public void Draw(DogModel model) {
      Console.WriteLine(model.Breed);
    }
  }

  class MainClass {
    public static void Main(string[] args) {
      // Works. Output is:
      // 9 Whiskers
      // collie
      CatModel cat = new CatModel();
      cat.NWhiskers = 9;
      DogModel dog = new DogModel();
      dog.Breed = "collie";
      List<IAnimalModel> animals = new List<IAnimalModel>(){ cat, dog };
      foreach (IAnimalModel animal in animals) {
        animal.Draw();
      }
    }
  }
}
\$\endgroup\$
3
\$\begingroup\$

You have the tightest coupling of interfaces I have seen in my life. You are also restricting them in abnormal manners. For example, IDrawer should not need to know about the IAnimalModel. Instead, it should be worried about IDrawableModel, which IAnimalModel can worry about. This way, the coupling is reduced and things are coupled with appropriate interfaces/classes. You want to reduce your coupling to only couple around what needs to depend on what else. Things that are generic like IDrawer shouldn't have to worry about more specific things. They should be, well, for the buzzword: generic.

Think about it as "what happens if I need something else to draw?" If you have another class/interface that has to draw something, and you need to modify IDrawer to count that in, then something is fishy.


You could test this better by developing around Test-Driven Development. You have interfaces for a reason, they are also very good at TDD. It's easy to mock things with them. (So that you can verify that whatever the thing is works. Especially with generics.)


Please do us all a favour and format your code more appropriately. C# does not use Egyptian braces (at least, not by any best-practices). (Java and JAVA do, but this is neither Java nor JAVA.)

namespace GenericInterface
{
    public interface IDrawer
    {
    }

    public interface IDrawer<TModel> : IDrawer
      where TModel : IAnimalModel<TModel>
    {
        void Draw(TModel model);
    }
\$\endgroup\$
  • \$\begingroup\$ -1. There's nothing wrong with OP's braces. They're consistent, which is more important than one true brace style. \$\endgroup\$ – RubberDuck Oct 6 '15 at 17:55
  • \$\begingroup\$ @RubberDuck I agree with consistency. But being consistently wrong is still being wrong ;-) \$\endgroup\$ – Mathieu Guindon Oct 6 '15 at 19:00
1
\$\begingroup\$

Considering the indentation and spacing your code has, it is very, should I emphasize very difficult to give an opinion on such a post. You should consider using more white spaces, or using this bracket style :

//Gives more space
class a
{
}

versus

class a{
}

One way or another, your code will be much more readable.

From what I understand, should IDrawer<T> be IAnimalDrawer<T> since there's a type constraint on T is IAnimalModel<T>?

Apart from that, I strongly believe it'd be easier to review with a class diagram, as it gives a better overview of the inheritance. But it seems like your inheritance model too complex. I'm not sure I see why there's a separation between GetDrawer and Draw. Try to take a look at covariance and contravariance, it might help you simplifying the hierarchy.

Really fix the spacing, then you could post a follow-up question so we can put up a better review .

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.