Incorrectly generated random list

Question

I want to generate a list of cars with random characteristics of make, model, color and year. These values are stored in a car list. The problem is, when it comes time to do the loop calling the function gerarCarro(), which adds random values to the car list, values from the third car start to be all the same.

using System;
using System.Collections.Generic;
using System.Linq;

public class MainClass
{
  public static List<string> marcaLista = new List<string>();
  public static List<string> modeloLista = new List<string>();
  public static List<string> corLista = new List<string>();

  public static List<string> carro = new List<string> ();

  public static void Main(string[] args)
  {
    marcaLista.Add("Toyota");
    marcaLista.Add("BMW");
    marcaLista.Add("Volkswagen");
    marcaLista.Add("GM");
    marcaLista.Add("Mercedes");

    modeloLista.Add("1.0");
    modeloLista.Add("1.4");
    modeloLista.Add("1.6");
    modeloLista.Add("1.8");
    modeloLista.Add("2.0");

    corLista.Add("Branco");
    corLista.Add("Prata");
    corLista.Add("Preto");
    corLista.Add("Cinza");
    corLista.Add("Vermelho");

    for (int i = 1; i <= 5; i++)
    {
      gerarCarro ();
      foreach (string item in carro)
      {
        Console.WriteLine (item);
      }
      Console.WriteLine ();

      carro.Clear ();
    }

  }

  public static void gerarCarro ()
  {
    int randomizer;

    Random random = new Random ();

    for (int i = 0; i < 4; i++)
    {
      randomizer = random.Next (0, 5);

      switch (i)
      {
        case 0:
          carro.Add (marcaLista[randomizer]);
          break;
        case 1:
          carro.Add (modeloLista[randomizer]);
          break;
        case 2:
          carro.Add (corLista[randomizer]);
          break;
        case 3:
          carro.Add (random.Next (2000, 2018).ToString());
          break;
      }
    }
  }
}

Answer 1

I know it’s not exactly the way you wanted it, but you learn to do it the right way. The main problem is that it was generating a new random seed every time it calls the method, and it ends up being the same. Seed must be generated only once in the application, and making it a member of the class causes this to occur.

I also organized and modernized the code. It got much leaner. I took what I didn’t need. I changed the array by a class that is how it is done in C#. I eliminated the loop (for) and the selection flow (switch) that generated elements because it only complicated the code without generating benefit. I made the car be returned so as not to need a class member for no reason.

You can still improve much more, but you learn right without having to learn too much.

Make code simple. It’s more readable, but easy to maintain, easier to understand. Get used to using C nomenclature standard#.

using System;
using static System.Console;
using System.Collections.Generic;

public class MainClass {
    public static Random Random = new Random();

    public static List<string> Marcas= new List<string>() { "Toyota", "BMW", "Volkswagen", "GM", "Mercedes" };
    public static List<string> Modelos = new List<string>() { "1.0", "1.4", "1.6", "1.8", "2.0" };
    public static List<string> Cores = new List<string>() { "Branco", "Prata", "Preto", "Cinza", "Vermelho" };

    public static void Main(string[] args) {
        for (int i = 1; i <= 5; i++) {
            var carro = gerarCarro();
            WriteLine(carro.Marca);
            WriteLine(carro.Modelo);
            WriteLine(carro.Cor);
            WriteLine(carro.Ano);
            WriteLine();
        }
    }
    public static Carro gerarCarro() {
        return new Carro() {
            Marca = Marcas[Random.Next(0, 5)],
            Modelo = Modelos[Random.Next(0, 5)],
            Cor = Cores[Random.Next(0, 5)],
            Ano = Random.Next(2000, 2018) };
    }
    public class Carro {
        public string Marca { get; set; }
        public string Modelo { get; set; }
        public string Cor { get; set; }
        public int Ano { get; set; }
    }
}

Behold working in the ideone. And in the .NET Fiddle. Also put on the Github for future reference.

If you want to avoid repeating the same characteristic you should not use random number but a algorithm Fisher-Yates.

Answer 2

Dude, I made a much simpler code, to help you with this problem. I still think it gives to improve more, mainly using the LINQ, but since I don’t know how to use it, I did it my way:

using System;
using System.Collections.Generic;

namespace CarroSO
{
    class Program
    {
        static List<Carro> carros = new List<Carro>(); //Crio varios array para utilizar melhor os valores
        static string[] marcas = { "Toyota", "BMW", "Volskwagen", "GM", "Mercedes" };
        static string[] modelos = { "1.0", "1.4", "1.6", "1.8", "2.0" };
        static string[] cores = { "Branco", "Prata", "Preto", "Cinza", "Vermelho" };

        static void Main(string[] args)
        {
            int valor;
            Random r = new Random();

            for (int i = 0; i < 5; i++)
            {
                do
                {
                    valor = r.Next(0, 5);
                } while (testarExistencia(modelos[valor])); //Checa se já existe o carro
                carros.Add(new Carro(marcas[valor], modelos[valor], cores[valor])); //Adiciona o carro, caso não exista
            }

            foreach (Carro car in carros) //Passa por todos os carros
                Console.WriteLine($"Marca: {car.marca} - Modelo: {car.modelo} - Cor: {car.cor}"); //Escreve cada um no console
        }

        static bool testarExistencia(string modelo) //Retorna true se já existe e false caso não exista
        {
            foreach (Carro carro in carros)
            {
                if (carro.modelo == modelo)
                {
                    return true;
                }
            }
            return false;
        }
    }

    class Carro
    {
        public string marca { get; set; }
        public string modelo { get; set; }
        public string cor { get; set; }

        public Carro(string marca, string modelo, string cor) //Constructor para criar um objeto do carro
        {
            this.marca = marca;
            this.modelo = modelo;
            this.cor = cor;
        }
    }
}

Notice that I mentioned it so I can explain a little bit of how it works.