WPF circle and line drawer solution

Question

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> 

Answer 1

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(); }