HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Car generation algorithm

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
algorithmgenerationcar

Problem

I have an algorithm which generates some cars (quote unquote) and displays them using Graphics. I would like to know if my code is using Random correctly, and if it has any memory leaks. Or if there is anything I did wrong and I should improve.

```
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace WindowsFormsApplication2
{
public partial class Form1 : Form
{
Graphics g;
public Random rand;
public Form1()
{
InitializeComponent();
rand = new Random();
}
private void button1_Click(object sender, EventArgs e)
{
g = ActiveForm.CreateGraphics();

int i = 0;
do
{
Car car = new Car();
Body body = GenerateBody();
car.body = body;
car.name = GenerateName();
car.tires = GenerateTires(body.edges.ToArray());
//DrawCar(car);
comboBox1.Items.Add(car);
i++;
} while (i edges = new List();
edges.Add(new PointF(256, 256)); //PointF of first edge of the polygon, so the body is created
do
{
PointF newPointF = new PointF(rand.Next(-60, 61) + edges.Last().X, rand.Next(-60, 61) + edges.Last().Y);
//Use last element to create a new polygon relative to last PointF, and change the coords by a random value
//between 1 and 20
edges.Add(newPointF);
i++;
} while (i tirelist = new List();
int i = 0;
do
{

//Create a random tire
CarTire t = new CarTire(Color.FromArgb(255, rand.Next(0, 256), rand.Next(0, 256), rand.Next(0, 256)), rand.Next(10, 20), edges[i]);
tirelist.Add(t);

i++;
} while (i edges = new List();
public Body(Color carColor,List bodyEdges)

Solution

g = ActiveForm.CreateGraphics();


This is bad. The old or previous Graphics object needs to be disposed before you create a new one but even so using a global Graphics object is still bad.

You should override the OnPaint method and do the drawing there.

protected override void OnPaint(PaintEventArgs e)
{
    var g = e.Graphics;

    // drawing...       
}


Pass the Graphics object to all drawing methods and call them from OnPaint

private void DrawCar(Graphics g, Car c)
{
    // draw the car..,
}


g.FillEllipse(new SolidBrush(t.color), r)


This is bad too. When working with Graphics nearly everything needs to be disposed so does the SolidBrush. The right way to use it is like this

using(var sb = new SolidBrush(t.color))
{
    g.FillEllipse(sb, r);
}


But not everything is bad. You wrote classes for the car, for the body and tires. Your methods have only a single responsibility each. This is all good. From the design point of view you now should put them in appropriate classes that have a single responsibility too similar to what @chenop suggested in his answer. You're on the right track.

Code Snippets

g = ActiveForm.CreateGraphics();
protected override void OnPaint(PaintEventArgs e)
{
    var g = e.Graphics;

    // drawing...       
}
private void DrawCar(Graphics g, Car c)
{
    // draw the car..,
}
g.FillEllipse(new SolidBrush(t.color), r)
using(var sb = new SolidBrush(t.color))
{
    g.FillEllipse(sb, r);
}

Context

StackExchange Code Review Q#152622, answer score: 6

Revisions (0)

No revisions yet.