7

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.

enter image description here

4
  • What's the purpose of these classes? Are they needed to hold and handle contents of documents or to render documents or both?
    – COME FROM
    Commented Mar 3, 2015 at 14:21
  • @COMEFROM for both.
    – Ahmad
    Commented Mar 5, 2015 at 8:30
  • Ok. I think you should follow Doval's and Doc Brown's advice and separate the task of rendering TextBlocks and Pages to other classes.
    – COME FROM
    Commented Mar 6, 2015 at 13:40
  • am curious why not just instantiate Graphics within the Draw() method - is there any reason you want to pass instantiate it in the Page class and pass it in as a parameter?
    – BenKoshy
    Commented May 27, 2017 at 2:50

3 Answers 3

9

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); 
   }
}
2
  • Thank you for the suggestions, And Yes, I may should have said it in the question that in TextBlock I have some functions which all need the graphics, for example Write (or Draw) and HightLight or OverWrite functions
    – Ahmad
    Commented Mar 3, 2015 at 7:47
  • Currently I used your first suggestion with one or two public methods for drawing which receive Graphics as input, and use the Graphics as a private member in the class
    – Ahmad
    Commented Mar 5, 2015 at 12:52
6

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.

0
4

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.

4
  • The Graphics object is a Winforms gfx device context, definitely not suitable for adding specific drawing logic to it. It provides a very stable interface, and it provides also an abstraction from the specific drawing device.
    – Doc Brown
    Commented Mar 3, 2015 at 14:50
  • 1
    @DocBrown I don't think that'd impede him from creating a function/class that takes pages and the Winforms context and takes care of drawing though.
    – Doval
    Commented Mar 3, 2015 at 14:52
  • 1
    Of course not, I was under the impression I already suggested exactly that to him in my answer (or at least partially, by refactoring the drawing logic out into a separate TextBlockDrawer class).
    – Doc Brown
    Commented Mar 3, 2015 at 16:18
  • First I drew Textblocks in Page itself, but I needed to keep some state for each block (last cursor position), Actually I render words as they are spoken by a Text to Speech engine, anyway to keep some stat of drawing I think I may need a drawing class for each TextBlock
    – Ahmad
    Commented Mar 5, 2015 at 12:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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