OOP Practice: how to best handle dependent dependencies

Question

I have a problem with the design shown in the picture. My Section class has some TextBlocks (or simply Blocks). The section should be drawn on a page (a Bitmap as device context). It sets the blocks of the page and calls page.DrawBlocks(). The page sets the Graphics object of each block and calls block.Draw()

Graphics could not be set in the constructor of each TextBlock when I introduce them in the Section because the page may not be initialized. However, I know it's better to introduce dependencies in the constructor.

How can I redesign my classes to solve this problem? Should I pass the Graphics to the Draw() function? I prefer not, because some other functions in the class also need Graphics.

Answer 1

Should I pass the Graphics to the Draw() function?

Well, when I saw this design, first thing which came into my mind was "why the heck does the TextBlock have a Graphics attribute at all? Should it not be possible to draw the same block on two different graphics contexts? Should the TextBlock object not have a lifetime which may be longer than the lifetime of the Graphics object?"

So my answer is yes - it does not make much sense to me not to have a Graphics parameter in the Draw function. This will imply to pass the Graphics parameter into the DrawBlocks method of the Page, and some other method in your Section, of course, but that is probably the correct design.

In fact, when your TextBlock has more than a half dozen private methods, all called directly or indirectly from Draw, and all of them use the same Graphics object, it may be more convenient to buffer the Graphics object passed by the Draw method in a member variable, so you do not need to have the same Graphics parameter in all of those methods. So keeping this attribute as a member might make sense (but this cannot be deciced from the small part of the text block class we see in your current model). And if you decide to keep this attribute, make sure you add a comment when this attribute is set and how long it is valid. The code then will look like this

   void Draw(Graphics g)
   {
      graphics=g;
      // ...
      // call some members depending on "graphics"
      // ...
      graphics=null; // just in case, to avoid accessing an invalid graphics object
   }

Alternatively, you could consider to introduce a helper class TextBlockDrawer, which gets the Graphics object and the TextBlock passed as a constructor parameter, and encapsulates the whole drawing process. That way, you won't need a Graphics member in your TextBlock class any more, only in TextBlockDrawer, and this attribute can be initialized in the constructor of that class. This design also makes a better separation between the data (TextBlock) and the drawing process. The resulting code will look either like this:

class TextBlock
{
   public void Draw(Graphics g)
   {
      var tbDrawer = new TextBlockDrawer(this,g);
      tbDrawer.Draw(); 
   }
}

or, by omitting the Drawmethod in the TextBlock completely, and reuse the TextBlockDrawer for different text blocks:

class Page
{
   // ...
   public void DrawBlocks(Graphics g)
   {
      var tbDrawer = new TextBlockDrawer(g);
      foreach(var tb in MyTextBlocks())
          tbDrawer.Draw(tb); 
   }
}

Answer 2

If the application uses C#, there is also an element not mentioned by Doc Brown which makes Graphics as property a bad choice (it may probably be relevant for Visual Basic and partially relevant for Java as well).

Graphics implements IDisposable. This means that if you pass it to Draw(), the code looks straightforward:

using (var g = new Graphics(...))
{
    foreach (var block in this.Blocks)
    {
        block.Draw(g);
    }
}

On the other hand, if you use a property, it becomes very difficult to:

• Determine inside the caller class when should you actually dispose the Graphics object,

• Find inside TextBlock class whether the Graphics object is already disposed.

You'll end up with something which looks like:

using (var g = new Graphics(...))
{
    foreach (var block in this.Blocks)
    {
        block.Graphics = g;
        block.Draw();
        block.Graphics = null;
    }
}

Do you use Code analysis? I would be surprised if you have zero errors with the actual approach.

Answer 3

Your problem is that TextBlocks shouldn't know how to draw themselves in the first place. This is a Single Responsibility Principle violation; now your classes will have to change any time the Graphics interface changes or any time you want to change how they get drawn to the screen, even though their primary concern should be representing parts of pages.

Instead, make Graphics be the one to draw pages and text blocks. This puts all the graphics logic in one place, lets you change it without affecting the rest of your code, and you don't have to pass around stub Graphics objects when you unit test.