Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

and here is the 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;
            }
        }
    }

and the XAML, too:

<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>

Thanks for the answers!

share|improve this question

1 Answer 1

up vote 2 down vote accepted

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();
}
share|improve this answer
    
thanks :) i'm learning interfaces now and it helped me a lot. :D –  mitya221 Feb 11 '13 at 17:22
    
Glad I could help. If there's something you don't understand, just ask! ;) –  Abbas Feb 12 '13 at 9:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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