patterncsharpMinor
Car generation algorithm
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)
```
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 OnPaintprivate 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 thisusing(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.