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
.
4
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 Draw
method 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
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 theGraphics
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
Your problem is that TextBlock
s 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