Car generation algorithm

Question

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

    private Body GenerateBody()
    {

        int amountOfEdges = rand.Next(2, 6); 
        int i = 0;
        List<PointF> edges = new List<PointF>();
        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 < amountOfEdges);
       //             
       //Give our car a random color
        Color randomColor = Color.FromArgb(200, rand.Next(0, 256), rand.Next(0, 256), rand.Next(0, 256));
        return new Body(randomColor, edges);
    }

    private CarTire[] GenerateTires(PointF[] edges)
    {
        int tires = rand.Next(1, edges.Length + 1); //Tire amount is random from 1 to edge length. Each tire has the position of the corresponding edge.
        List<CarTire> tirelist = new List<CarTire>();
        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 < tires);
        return tirelist.ToArray();
    }
    private string GenerateName()
    {
        int length = rand.Next(5, 12);
        int i = 0;
        StringBuilder sb = new StringBuilder();
        string charset = "abcdefghijklmnopqrstuwvxyz";
        do
        {
            sb.Append(charset[rand.Next(0, charset.Length)]);
            i++;
        } while (i < length);
        return sb.ToString();
    }
    private void DrawCar(Car c)
    {
        //Draw tires first, so body is on top

        foreach (CarTire t in c.tires)
        {
                Rectangle r = getCircle(t.location, t.radius);
                g.FillEllipse(new SolidBrush(t.color), r);

        }

        //Draw the body
        g.FillPolygon(new SolidBrush(c.body.color), c.body.edges.ToArray());

    }
    private Rectangle getCircle(PointF center, int radius)
    {
        return new Rectangle((int)Math.Round(center.X - radius),(int)Math.Round(center.Y - radius), radius * 2, radius * 2);

    }
    class Car
    {

        public override string ToString()
        {
            return name;
        }

        //A car needs tires and a body.
        public Body body;
        public CarTire[] tires;
        public string name;     
    }
    class Body
    {
        public Color color;
        public List<PointF> edges = new List<PointF>();
        public Body(Color carColor,List<PointF> bodyEdges)
        {
            color = carColor;
            edges = bodyEdges;
        }
    }
    class CarTire
    {
        public Color color;
        public int radius;
        public PointF location;
        public CarTire(Color tirecolor, int tireradius,PointF position)
        {
            color = tirecolor;
            radius = tireradius;
            location = position;
        }
    }
    private void comboBox1_SelectedIndexChanged(object sender, EventArgs e)
    {
        g.Clear(ActiveForm.BackColor);
        DrawCar((Car)comboBox1.SelectedItem);


        }
}
}

Button1 is a button with label "Generate some cars", it generates some cars and puts them in the combo box.

Here is a gif of the cars generated:

Answer 1

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.

Answer 2

I would expect separating the UI interactions, the drawing actions and the generating car actions.

• AppManager - The Application Manager
• CarGenerator - The class which resposible to generate a car and its properties
• DrawingManager - The class which responsible for drawing something, it is the only class that consume the graphics class.

So I suggest the following:

Answer 3

Default of Random is 0

rand.Next(0, 6) same as rand.Next(6)

Can initialize like this public Random rand = new Random();

I think this is cleaner for a loop

for (int i = 0; i < 20; i++)
{
}

Public is typically Capital