|
Sniped!
I think you actually don't want to dispose the pen in this case, though, because it's being read out of a global variable which is presumably caching it.
(Also, the brush is different for the two cases, like the pen.)
|
|
|
|
|
I missed the brushes case, and you're right - the Pen shouldn't be disposed. For some reason I thought I'd newed it.
|
|
|
|
|
I don't like it because it prefers Brushes.Transparent over Brushes.Black. Both brushes are equal. Here, if switch goes to the default case, then Brushes.Transparent would be used despite there isn't any reason to use it and not Brushes.Pink. The same with radius factors -- factor '1.2' isn't more special than '1'.
Greetings - Jacek
|
|
|
|
|
Fair point, but you could simply exit out in the default case (I've never been a fan of default in switch statements - it's always seemed that it should be for the error case).
|
|
|
|
|
Pete O'Hanlon wrote: Fair point, but you could simply exit out in the default case
Oh these sexy quotation boxes.
Well, it still visually suggests that Transparent is a default value, because brush is assigned twice. Oh and redutant assignment == wasted CPU cycles.
Greetings - Jacek
|
|
|
|
|
I rather agree – see my post where I used the default for a throw. When you're switching on an enum it's usual that you have defined behaviour for all cases and anything unexpected should throw an exception.
|
|
|
|
|
I prefer the second, but I'd take the ellipse drawing out of the switch, since the logic is (to me) 'set up parameters depending on head type, then draw it' and the last part is common. So I'd do:
protected virtual void RenderHead(DrawingContext drawingContext, NoteHeadTypes type)
{
double y = NoteVerticalPosition;
double xRadiusFactor, yRadiusFactor;
Brush brush;
Pen pen;
switch (type)
{
case NoteHeadTypes.Filled:
yRadiusFactor = 1.1;
xRadiusFactor = 1.2;
brush = Brushes.Black;
pen = NoteHeadPen;
break;
case NoteHeadTypes.Empty:
yRadiusFactor = xRadiusFactor = 1;
brush = Brushes.Transparent;
pen = NoteBeamPen;
break;
default:
throw new ArgumentException("Unknown head type");
}
double xRadius = HeadXRadius * xRadiusFactor,
yRadius = HeadYRadius * yRadiusFactor;
drawingContext.DrawEllipse(brush, pen, xRadius / 2 + XOffset + 4.5, y, xRadius - 1, yRadius);
}
|
|
|
|
|
This is good, I like explicitness of all assignments, like brush, pen and xyRadiusFactor. DrawEllipse may change individually for each method, but this solution makes future refactoring easy.
Greetings - Jacek
|
|
|
|
|
I'd personally go for option 1, I'm of the opinion that switches are evil, but everytime I express this view I get downvoted. Rather than explain why here, have a look at SwitchStatementsSmell[^].
If you can't alter your class design much, you could consider this option (removing duplication under the "DRY" principle) :
virtual void RenderHead(DrawingContext drawingContext,
Brush brush = Brushes.Transparent,
Pen pen = NoteBeamPen,
decimal xScale =1,
decimal yScale =1)
{
double y = NoteVerticalPosition;
double yRadius = HeadYRadius * yScale;
double xRadius = HeadXRadius * xScale;
drawingContext.DrawEllipse(brush, pen, new Point(xRadius / 2 + XOffset + 4.5, y), xRadius - 1, yRadius);
}
protected virtual void RenderHead(DrawingContext drawingContext, NoteHeadTypes type)
{
if(type==NoteHeadTypes.Filled)
RenderHead(drawingContext, Brushes.Black, NoteHeadPen, 1.2, 1.1);
else if(type==NoteHeadTypes.Empty)
RenderHead(drawingContext);
}
Note this code only works with .net 4 (for brevity), but the named arguments can easily be refactored back.
|
|
|
|
|
I prefer to remove the switch smell, but sometimes it's overkill. In all things there should be balance.
|
|
|
|
|
Sorry, but it's all bad.
1. First of all, it doesn't compile because neither NoteBeamPen nor Brushes.X are constant.
2. It is not clear what 'factors' RenderHead(drawingContext) are. I don't see why factor '1' should be 'more default' than, e.g. 0 or 10 or 1.5 or whatever.
3. Additionally, drawingContext.DrawEllipse is not actually redutant code. It can change to 'DrawGeometry' or other stuff in future, and then it would require refactoring if different geometry was used in these methods. And, using your solution refactoring wouldn't be straightforward.
4. You have replaced switch with ifs which doesn't actually change devilness.
Greetings - Jacek
modified on Tuesday, June 28, 2011 7:11 AM
|
|
|
|
|
Jacek Gajek wrote: 1. First of all, it doesn't compile because neither NoteBeamPen nor Brushes.X are constant.
I didn't copile it as I don't know which namespaces you are using, just describing the princple. Perhaps I should have made this clear.
Jacek Gajek wrote: 2. It is not clear what 'factors' RenderHead(drawingContext) are. I
don't see why factor '1' should be 'more default' than, e.g. 0 or 10 or 1.5 or
whatever.
This defaults to the "empty" version, but that is all they are defaults. You could default to whatever you feel safest, but defaulting to the one that scales at 1 makes the most sense to me.
For both the points above,you can drop the defaults totally and pass these in makes you feel any better, without altering the key point I made.
Jacek Gajek wrote: Additionally, drawingContext.DrawEllipse is not actually redutant code. It can
change to 'DrawGeometry' or other stuff in future, and then it would require
refactoring if different geometry was used in these methods.
I didn't say that was redundant, just that you have repeated code, as other posters have pointed this out to you too. Further refactoring is unlikely be a huge chore, and you'd only need to do code on one place most probably. In any case I also question the wisdom of future-proofing against "geometry changes" unless they are reasonably certain to change. On this last point, if these are likely to change, how we we be able to tell, answer: we can't wihtout you letting us know your requirements.
|
|
|
|
|
Keith Barrow wrote:
I didn't copile it as I don't know which namespaces you are using, just describing the princple. Perhaps I should have made this clear.
Well they cannot be constant unless it was a 'null' constant.
I do not agree with an idea of defining "default" values, even if their usage is optional. In my opinion, method signature should force developer to explicitely assign parameters. And it is not all about providing more precise requirements. But well, you are impressively good in defending your ideas, 5 for this 
Greetings - Jacek
|
|
|
|
|
Counter-5 for being a good sport!
|
|
|
|
|
If switch statements 'smell', then
if(thing == option1) ...
else if(thing == option2) ...
... also smells, and (unless it's a type for which switch is not supported in the language) it is worse than doing the switch explicitly because it's harder to see that that's what you're doing.
The rule of thumb is to do with duplication. If he's switching on this enum in several places, perhaps the class hierarchy should be adjusted so that it is not an enum, it is a class tree with a Render method (and methods for the other operations that are being switched on). There isn't really any evidence to suggest that that is the case in the original post, though, and it's somewhat outside the scope of the question that was asked.
|
|
|
|
|
BobJanova wrote: also smells
Agreed, but where I'd disagree is here:
BobJanova wrote: because it's harder to see that that's what you're doing.
I'd say having the condition visible at each branch is more clear, but that is a matter of opinion.
BobJanova wrote: If he's switching on this enum in several places, perhaps the class hierarchy should be adjusted so that it is not an enum
Yes, and the article I pointed him to makes this very point. As I said, he might not have the luxury of altering that much (the graphics classes are riddled with enums, forcing attendant switches), so I gave him another option.
|
|
|
|
|
About the second point: the reason the switch is clearer is because you can see immediately when you see the word 'switch' that it is going to be a selection based on a single value. With the if/else there's nothing to stop you doing
if(c == ' ') ...
else if(c == '/') ...
else if(c == '^' && accountBalance > 50) ...
else if(c == '&') ...
... so you have to read each line carefully to know what the logic is actually doing.
|
|
|
|
|
I demonstrate using Action to perform functionality instead of using a switch (or an if/else condition) in this[^] blog post.
|
|
|
|
|
BobJanova wrote: class tree with a Render method
I have considered it but it has prons and cons too -- see my answer to "SledgeHammer01"[^].
Greetings - Jacek
|
|
|
|
|
3. Repeating drawingContext.DrawEllipse is not as redutant as it looks like. Different methods might be used in future, e.g. DrawEllipse in the first method and DrawGeometry in the second one.
4. It's WPF, Pen doesn't have to be explicitely disposed.
Greetings - Jacek
modified on Tuesday, June 28, 2011 6:59 AM
|
|
|
|
|
Second, kinda. Edit: The second is the better of those two.
3) Have a NoteHeadInfo class (or structure) that contains the sizing factor and a reference to the pen and pass one in.
3.a) Have a Dictionary<NoteHeadTypes,NoteHeadInfo> and use it in your second example.
modified on Tuesday, June 28, 2011 9:46 PM
|
|
|
|
|
The second for a trillion reasons:
1) the first method requires you to do a switch / if-then EVERYWHERE you call the functions so you can decide which one to call... the second method doesn't and its more self standing
2) you don't have any more HeadTypes NOW... and yes, I agree, music is probably not going to add another head type anytime soon, but the first method would require you to write a 3rd function and update every piece of calling code that decided whether to call the first two functions while the second method would only require you to update the single function and none of the calling code.
|
|
|
|
|
Both points aren't suitable to my specific scenario.
No, because this is from a NoteVisual class, which is base for e.g. QuarterNoteVisual. Every child class (quarter, half, full etc) use just one of these methods and does not do any switch.
It is possible to design a deeper inheritance
Note
abstract ~RenderHead
FilledNote
+RenderHead
QuarterNote
and
Note
abstract ~RenderHead
EmptyNote
+RenderHead
HalfNote, FullNote
But this would make the code too javaish -- superdeep inheritance which quickly become hard to maintain.
Greetings - Jacek
|
|
|
|
|
Ah, ok, so if I'm to understand, its more like:
HalfNote.Render -> call base.Func1()
QuaterNote.Render -> call base.Func2()
In that case, I'd *still* say the 2nd method is preferable
1) you have one base function to maintain vs two (or more)
2) base.Func(HeadTypes.Filled) and base.Func(HeadTypes.Unfilled) looks and feels cleaner to me
On the other hand, if you are drawing a *lot* (like 10's of thousands) of these notes and need to squeeze out every single iota of performance then the first method would probably be better since you can get rid of the switch and its associated jumps. From the code you posted, you have a *bit* of room for other improvements too, but getting rid of the switch for performance reasons will only come into play if you are calling these functions a *lot* (like 10's of thousands of times).
|
|
|
|
|
I agree with you in general, I wanted to make some adnotations to your post. BTW, I think that it would have to be *much more* than 10's of thousands to observe a differnce between switch vs override. My post was about coding style, not performance. BTW max number of calls will be ~50.
Greetings - Jacek
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
