-1
\$\begingroup\$

This is my attempt at simulating an eco system. Basically this thing is suppost to simulate how an eco system works. Its not the most accurate thing but it's something. Let me know what you think about it.

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

namespace Ecosystemsimulation
{
    
    class Rabbit
    {
        public bool Haseaten   // property
        {
            get; set;
        }
        public Rabbit(bool hasalreadyeaten)
        {
            Haseaten = hasalreadyeaten;
        }
    }
    class Program
    {
        public static bool GenerateRandombool(int precentage)
        {
            var rand = new Random();

            if (rand.Next(1000) < precentage * 10)
            {
                return true;
            }
            return false;
        }
        static void Main(string[] args)
        {
            Console.WriteLine("Ecosystem Simulation");
            Console.WriteLine("---Enter the amount of prey---");
            int RabbitsAmount = Int32.Parse(Console.ReadLine());
            List<Rabbit> rabbits = new List<Rabbit>();
            for (int i = 0; i < RabbitsAmount; i++)
            {
                rabbits.Add(new Rabbit(false));
            }
            Console.WriteLine("---Enter the amount of avialable crops per day---");
            int Foodavailable = Int32.Parse(Console.ReadLine());
            Console.WriteLine("---Write the amount of days to siumlate---");
            int Daysamount = Int32.Parse(Console.ReadLine());
            //Console.WriteLine("---Write the amount of predetors ---");
            //int predator = Int32.Parse(Console.ReadLine());
            Console.WriteLine("---Write the probabilty of repuduction---");
            int reproductionperc = Int32.Parse(Console.ReadLine());
            int dead = 0;
            Console.WriteLine("------------------Starting Simulation------------------");
            int Foodavailablefortheday = Foodavailable;
            int deathstoday = 0;
            for (int i = 0; i < Daysamount; i++)
            {
                foreach (Rabbit obj in rabbits.ToList())
                {
                    if (!obj.Haseaten)
                    {
                        if (Foodavailablefortheday > 0)
                        {
                            obj.Haseaten = true;
                            Foodavailablefortheday = Foodavailablefortheday - 1;
                        }
                    }
                }
               
                /*if (GenerateRandombool(reproductionperc))
                {
                    rabbits.Add(new Rabbit(true));
                }*/
                foreach (Rabbit obj in rabbits.ToList())
                {
                    if (!obj.Haseaten)
                    {
                        dead = dead + 1;
                        deathstoday = deathstoday + 1;
                        rabbits.Remove(obj);
                    }
                }
                if (rabbits.Count % 2 == 0 && rabbits.Count != 0 && rabbits.Count != 1)
                {
                    int Rabbitcountrepu = rabbits.Count / 2;
                    for (int u = 0; u != Rabbitcountrepu; u++)
                    {
                        rabbits.Add(new Rabbit(true));
                    }
                }
                else if(rabbits.Count >= 3)
                {
                    int Rabbitcountrepu = rabbits.Count - 1;
                    Rabbitcountrepu = reproductionperc / 2;
                    for (int u = 0; u != Rabbitcountrepu; u++)
                    {
                        rabbits.Add(new Rabbit(true));
                    }
                }
                foreach (Rabbit obj in rabbits.ToList())
                {
                    obj.Haseaten = false;
                }
                Console.WriteLine($"Deaths total day {i}:{deathstoday}" );
                Console.WriteLine($"Alive total day {i}:{rabbits.Count}");
                Console.WriteLine("------------------");
                Foodavailablefortheday = Foodavailable;
                deathstoday = 0;
            }
            int alive = 0;
            foreach (Rabbit obj in rabbits.ToList())
            {
                alive = alive + 1;
            }
            Console.WriteLine("------------------Simulation over------------------");
            Console.WriteLine("We ended up with " + dead + " casualt(ies)(y)");
            Console.WriteLine("We ended up with " +alive+ " Still alive");
            Console.WriteLine($"Amount of creatures who lived once { alive + dead}");

        }
    }
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ One quick tip: be consistent with your casing. Should have "HasEaten", "GenerateRandomBool", "hasAlreadyEaten" and there are others that seem a bit haphazard. Read docs.microsoft.com/en-us/dotnet/csharp/fundamentals/… for guidelines. \$\endgroup\$ – Jesse C. Slicer May 23 at 18:52
  • \$\begingroup\$ Thanks for your feedback \$\endgroup\$ – user242558 May 23 at 21:22
  • \$\begingroup\$ Sorry to say but this has nothing to do with OOP. \$\endgroup\$ – Peter Csala May 25 at 8:19
1
\$\begingroup\$

The biggest code smell I see is repeated enumeration of the same collection multiple times in a row, except you are actually creating copies for no good reason (more on this later).

Try to find a way to change your design to require enumerating a collection the least amount of times possible. This will improve performance and should also make it more clear to read and edit. Especially important as you scale up (enumerating a huge collection multiple times is not fast).

First one simple suggestion:

//Explicitly mark the accessibility of the class here.  Just good practice.
//Private, protected, public, internal, etc..
public class Rabbit
{
    //...SNIP...
}

I also agree with the comment about picking a consistent naming convention. Otherwise a variable like Haseatencarrotstoday is very hard to read.

Which casing is really opinion based, however I think the consensus is pick a naming convention and stick with it throughout.

Now to the worst part of the code:

foreach (Rabbit obj in rabbits.ToList())

Why ToList? rabbits is already a list... I would wager that's a workaround for modifying a collection during enumeration.

What's really happening is you're copying an array (lists are backed by an array) repeatedly simply due to bad design. That's what I'd call a hack. And really bad for performance, especially because it's entirely unnecessary.

Here's a better way (without modifying your Rabbit class, because I prefer a HashSet<T> here, but I digress):

var removeThese = new List<Rabbit>();
foreach (Rabbit obj in rabbits)
{
    if(<cwhatever condition to check for removal>)
    {
        removeThese.Add(obj);
        //Going to remove this, let's continue
        continue;
    }
    //Other stuff here
}
foreach(var remove in removeThese)
    rabbits.Remove(remove);

While that can be improved upon I think it demonstrates the problem and possible solutions.

Lastly, conditionals are evaluated left to right. So you want the fastest conditionals to be on the left, and more expensive on the right.

For example, changing

if (rabbits.Count % 2 == 0 && rabbits.Count != 0 && rabbits.Count != 1)

To

if (rabbits.Count > 1 && rabbits.Count % 2 == 0)

Nets better performance because checking Count > 1 is faster than using the mod operator %.

Sure, not a big difference in this case, but this can have tremendous impact if you can short circuit the condition as fast as possible. It's vital to understand this concept when writing complex conditionals.

It's extremely common to see things like null and count checks at the beginning of conditionals for this reason.

\$\endgroup\$
1
\$\begingroup\$

If the program simulates an ecosystem, I'd probably have an Ecosystem class.

Since weather may be a factor, for crops I might have a min and max that can be available each day.

My high level approach might be something like this:

var prey = new Rabbits(50);
var predators = new Predators(5);
var crops = new Crops(5, 10);
var days = 200;
var system = new Ecosystem(prey, predators, crops, days);
system.Evolve();
\$\endgroup\$
0
\$\begingroup\$

There quite a few problems with your code, here's a few of the worst ones:

  • You're instantiating a new Random object every time you create a new number. That is bad because the random object gets seeded on construction, and calling this thing multiple times quickly in a row will most likely give you the same value repeated. Plus it's expensive.

  • Repeated ToList() calls when iterating objects. That is terribly inefficient and absolutely unneeded.

  • Imagine someone gave you the piece of code new Rabbit(false) and nothing else. What does it mean? What do you mean you don't know, the code can't be so badly written that an object construction isn't immediately obvious what it does! There is nothing more important than knowing exactly why you are filling up your memory and with exactly what.

  • // property -- there are memes about people who write "comments" like those. Refer to the previous point to give you an idea of where you should be spending your efforts.

  • Haseaten is not how properties should be named in .Net, it should be HasEaten. There are very clear guidelines on naming: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/naming-guidelines

  • And lastly, if you reference the C# static analysis package, most of your code will light up green (or ghost out of existence) because it's using improper names (Int32), repeated names (new List<Rabbit>()), wasteful casts (Rabbit), wasteful Linq operations (ToList()), wrong operators (dead = dead + 1;), unnecessary assignments (int Rabbitcountrepu = rabbits.Count - 1) etc.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy