Problem
I’ve created a WPF program which can draw lines and circles on a canvas. It works well, but I don’t think it’s the best solution. How can I make it work better? How would you create this program?
MainWindow class:
public partial class MainWindow : Window
{
DrawCircle dc;
DrawLine dl;
public MainWindow()
{
InitializeComponent();
}
private void cVas_MouseMove(object sender, MouseEventArgs e)
{
switch (rbLine.IsChecked)
{
case true:
if (dl != null)
dl.MouseMove(Mouse.GetPosition(cVas));
break;
default:
if (dc != null)
dc.MouseMove(Mouse.GetPosition(cVas));
break;
}
}
private void cVas_MouseDown(object sender, MouseButtonEventArgs e)
{
switch (rbLine.IsChecked)
{
case true:
dl = new DrawLine(this);
dl.MouseDown(Mouse.GetPosition(cVas));
break;
default:
dc = new DrawCircle(this);
dc.MouseDown(Mouse.GetPosition(cVas));
break;
}
}
private void cVas_MouseUp(object sender, MouseButtonEventArgs e)
{
dc = null;
dl = null;
}
private void Window_KeyDown(object sender, KeyEventArgs e)
{
if (e.Key == Key.Enter)
cVas.Children.Clear();
}
}
DrawLine class (the DrawCircle is similar to this):
class DrawLine
{
private Line myLine;
MainWindow mw;
public DrawLine(MainWindow mw)
{
this.mw = mw;
}
public void MouseDown(Point mousePoint)
{
myLine = new Line();
myLine.Stroke = Brushes.Black;
myLine.StrokeThickness = 2;
myLine.X1 = myLine.X2 = mousePoint.X;
myLine.Y1 = myLine.Y2 = mousePoint.Y;
mw.cVas.Children.Add(myLine);
}
public void MouseUp()
{
myLine = null;
}
public void MouseMove(Point mousePoint)
{
if (myLine != null)
{
(mw.cVas.Children[mw.cVas.Children.IndexOf(myLine)] as Line).X2 = mousePoint.X;
(mw.cVas.Children[mw.cVas.Children.IndexOf(myLine)] as Line).Y2 = mousePoint.Y;
}
}
}
XAML:
<Window x:Class="WPFdraw.MainWindow"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
Title="MainWindow" Height="350" Width="525" KeyDown="Window_KeyDown">
<Grid>
<Grid.RowDefinitions>
<RowDefinition Height="*"></RowDefinition>
<RowDefinition Height="20"></RowDefinition>
</Grid.RowDefinitions>
<Canvas Grid.Row="0" Background="Transparent" Name="cVas" Width="Auto" Height="Auto" MouseMove="cVas_MouseMove" MouseDown="cVas_MouseDown" MouseUp="cVas_MouseUp"></Canvas>
<Grid Grid.Row="1">
<Grid.ColumnDefinitions>
<ColumnDefinition></ColumnDefinition>
<ColumnDefinition></ColumnDefinition>
<ColumnDefinition></ColumnDefinition>
</Grid.ColumnDefinitions>
<RadioButton Grid.Column="0" GroupName="tool" IsChecked="True" Name="rbLine" VerticalAlignment="Center">Line</RadioButton>
<RadioButton Grid.Column="1" GroupName="tool" Name="rbCircle" VerticalAlignment="Center">Circle</RadioButton>
<Button Name="btn" Grid.Column="2" Content="klikk" ></Button>
</Grid>
</Grid>
</Window>
Solution
A little explanation first. I have written your code in a different way. This does not mean that my code is perfect in any way. I just wanted you to see other options and possibilities to achieve the same result. I’m open for comments if supported by valid arguments.
First of all I suggest you use another naming convention. This way, the name of your controls and variables have a better meaning. For example ‘cVas’, I’d rename that to ‘DrawingCanvas’ or ‘DrawingSurface’.
Then there’s the structure of your code. You have different shapes that have a similar structure and share a certain functionality. In situations like that, you can start thinking about inheritance and/or the use of interfaces.
Below you’ll see the code that I have written. Again, this code is not perfect but it shows how it can be done. And to be honest: I think my code is easier to read, shorter and also easier to expand.
I have used an interface, from which both class derive. This way it was easy to create a Pen-object that will draw an IDrawable object. Using the Pen-object also de-couples your UI from the shape-objects. In the constructor you pass the UIElement (in this code only Canvas can be passed through) on which the pen will draw: they are coupled but in this case this is needed.
Due to the fact of the interface and pen-object, the code in your MainWindow.xaml.cs will also be a lot cleaner. You only need to instantiate one IDrawable-object and depending on the mouse-actions you provide what is needed.
I hope you find this useful and helpful! 😉
XAML:
I have not changed much about the XAML except for the names of the controls and I used a StackPanel instead of a Grid with columns.
IDrawable
public interface IDrawable
{
void Draw(Point location);
}
MyCircle
//Since you didn't provide the code of your circle-class
//I quickly wrote a dummy-circle class, it works but not
//flawless (X and Y coordinates)
public class MyCircle : IDrawable
{
public Ellipse Circle { get; private set; }
public MyCircle(Point location)
{
Circle = new Ellipse
{
Stroke = Brushes.Black,
StrokeThickness = 2,
Margin = new Thickness(location.X, location.Y, 0, 0)
};
}
public void Draw(Point location)
{
if(Circle != null)
{
Circle.Width = location.X - Circle.Margin.Left;
Circle.Height = location.Y - Circle.Margin.Top;
}
}
}
MyLine
public class MyLine : IDrawable
{
public Line Line { get; private set; }
public MyLine(Point location)
{
Line = new Line
{
Stroke = Brushes.Black,
StrokeThickness = 2,
X1 = location.X,
X2 = location.X,
Y1 = location.Y,
Y2 = location.Y
};
}
public void Draw(Point location)
{
Line.X2 = location.X;
Line.Y2 = location.Y;
}
}
Pen
public class Pen
{
public Pen(Canvas holder)
{
_holder = holder;
}
private readonly Canvas _holder;
public void Down(IDrawable obj)
{
//if more shapes are added:
//better use a switch-statement
if(obj is MyCircle)
_holder.Children.Add((obj as MyCircle).Circle);
if(obj is MyLine)
_holder.Children.Add((obj as MyLine).Line);
}
public void Draw(IDrawable obj, Point location)
{
obj.Draw(location);
}
}
MainWindow
public MainWindow()
{
InitializeComponent();
KeyDown += MainWindowKeyDown;
DrawingSurface.MouseMove += DrawingSurfaceMouseMove;
DrawingSurface.MouseDown += DrawingSurfaceMouseDown;
DrawingSurface.MouseUp += DrawingSurfaceMouseUp;
_pen = new Pen(DrawingSurface);
}
private IDrawable _dr;
private readonly Pen _pen;
private void DrawingSurfaceMouseUp(object sender, MouseButtonEventArgs e)
{
_dr = null;
}
private void DrawingSurfaceMouseDown(object sender, MouseButtonEventArgs e)
{
if(LineRadioButton.IsChecked == true)
_dr = new MyLine(Mouse.GetPosition(DrawingSurface));
else if(CircleRadioButton.IsChecked == true)
_dr = new MyCircle(Mouse.GetPosition(DrawingSurface));
_pen.Down(_dr);
}
private void DrawingSurfaceMouseMove(object sender, MouseEventArgs e)
{
if(_dr != null)
_pen.Draw(_dr, Mouse.GetPosition(DrawingSurface));
}
private void MainWindowKeyDown(object sender, KeyEventArgs e)
{
if(e.Key == Key.Enter)
DrawingSurface.Children.Clear();
}