Wa-Tor simulation

Question

I tried this as a programming exercise. It's working as intended, but I feel like the code is a mess and I could have used the advantages of object oriented programming much more (like inheritance for fish and shark from the Cell class, and probably much more). Can someone guide me a bit through this code example and show me how to make this more object-oriented?

The code simulates an ocean populated by fish and sharks:

• Sharks hunt fish
• Fish swim and breed
• Sharks die of starvation after x years

The ocean, fish and sharks are all represented by a Cell class. The objects are stored in a 2D array and then painted onto the Windows form.

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;
using System.Diagnostics;
using System.Threading;
namespace WaTor
{
public partial class Form1 : Form
{
 //Gritsize width(x) and height(y)
 const int TableX = 10;
 const int TableY = 10;
 const int pixelSizeX = 19;
 const int pixelSizeY = 19;
 //Fish Properties
 int initialNumberOfFish = 2;
 int initialNumberOfSharks = 2;
 int ageToBreedFish = 5;
 int ageToBreedShark = 10;
 int yearToStarvation = 5;
 int sharkScent = 3;
 Random rnd = new Random();
 Cell[,] myArray = new Cell[TableX, TableY];
 public Form1()
 {
 InitializeComponent();
 intitializeArray();
 }
 private void intitializeArray()
 {
 int x = 0; //Position on X-Axis
 int y = 0; //Position on Y-Axis
 for (int i = 0; i < myArray.GetLength(0); i++)
 {
 for (int j = 0; j < myArray.GetLength(1); j++)
 { 
 Cell newCellX = new Cell();
 myArray[i,j] = newCellX;
 myArray[i,j].posX = x;
 myArray[i,j].posY = y;
 x = x + pixelSizeX+1;
 }
 x = 0;
 y = y + pixelSizeY+1;
 }
 }
 //Only used to paint the results
 private void Canvas_Paint(object sender, PaintEventArgs e)
 {
 Graphics g = Canvas.CreateGraphics();
 for (int i = 0; i < myArray.GetLength(0); i++)
 {
 for (int j = 0; j < myArray.GetLength(1); j++)
 {
 Brush b = new SolidBrush(myArray[i,j].cellColor);
 Rectangle r = new Rectangle(myArray[i,j].posX, myArray[i,j].posY, pixelSizeX, pixelSizeY);
 g.FillRectangle(b, r);
 }
 }
 }
 //Only used when initializing a new simulation
 public void populate()
 {
 //Clean the grid
 for (int i = 0; i < myArray.GetLength(0); i++)
 {
 for (int j = 0; j < myArray.GetLength(1); j++)
 {
 kill(i, j);
 }
 }
 //Distribute Fish randomly
 int PosX=0;
 int PosY=0;
 bool isEmpty=false;
 Random rndX = new Random();
 Random rndY = new Random();
 PosX = rndX.Next(0, TableX);
 for (int i = 0; i < initialNumberOfFish; i++)
 {
 while (!isEmpty)
 {
 PosX = rndX.Next(TableX);
 PosY = rndY.Next(TableY);
 if (myArray[PosX, PosY].fishType == fishTypeEnum.empty)
 isEmpty = true;
 }
 myArray[PosX, PosY].fishType = fishTypeEnum.fish;
 myArray[PosX, PosY].cellColor = Color.Green;
 myArray[PosX, PosY].age = rndX.Next(0, ageToBreedFish);
 isEmpty = false;
 }
 //Distribute Sharks randomly
 for (int i = 0; i < initialNumberOfSharks; i++)
 {
 while (!isEmpty)
 {
 PosX = rndX.Next(0, TableX);
 PosY = rndY.Next(0, TableY);
 if (myArray[PosX, PosY].fishType != fishTypeEnum.shark)
 isEmpty = true;
 }
 myArray[PosX, PosY].fishType = fishTypeEnum.shark;
 myArray[PosX, PosY].cellColor = Color.Red;
 myArray[PosX, PosY].age = rndX.Next(0, yearToStarvation);
 myArray[PosX, PosY].ageStarvation = myArray[PosX, PosY].age;
 isEmpty = false;
 }
 }
 //A tick is a 'round' where fish swim and breed, sharks hunt and breed
 public void tick()
 {
 bool reproduce;
 for (int i = 0; i < myArray.GetLength(0); i++)
 {
 for (int j = 0; j < myArray.GetLength(1); j++)
 {
 //If Cell is Shark and hasent moved
 if ((myArray[i, j].fishType == fishTypeEnum.shark) && (!myArray[i, j].moved))
 { 
 //Sharks starve
 Debug.WriteLine("Hai alter"+myArray[i,j].age);
 if (myArray[i, j].ageStarvation > yearToStarvation)
 {
 kill(i, j);
 Debug.WriteLine("Hai verhungert");
 }
 //If not starved shark hunts
 hunt(i, j); 
 }
 else
 //If Cell is Fish and hasent moved
 if ((myArray[i, j].fishType == fishTypeEnum.fish) && (!myArray[i, j].moved))
 {
 reproduce = false;
 if (myArray[i,j].age >= ageToBreedFish)
 {
 reproduce = true;
 }
 move(i, j, fishTypeEnum.fish, reproduce);
 }
 }
 }
 //End Tick
 for (int i = 0; i < myArray.GetLength(0); i++)
 {
 for (int j = 0; j < myArray.GetLength(1); j++)
 {
 myArray[i, j].moved = false;
 }
 }
 }
 private void kill(int x, int y)
 {
 myArray[x, y].age = 0;
 myArray[x, y].fishType = fishTypeEnum.empty;
 myArray[x, y].cellColor = Color.Blue;
 myArray[x, y].moved = false;
 }
 private void hunt(int x, int y)
 {
 Random rnd = new Random();
 int direction = 0;
 int newX = x;
 int newY = y;
 bool moved = false;
 //Check for Fish nearby
 direction = fishNearBy(x, y, sharkScent);
 Debug.WriteLine("direction: " + direction);
 if (direction == 0)
 direction = rnd.Next(1, 5); 
 {
 switch (direction)
 {
 case 1://East. If Index 0 move to TableEnde
 if (newY > 0)
 {
 newY = newY - 1;
 moved = true;
 }
 else
 {
 newY = TableY - 1;
 moved = true;
 }
 break;
 case 2://West. If TableEnde move to 0.
 if (newY < TableY - 1)
 {
 newY = newY + 1;
 moved = true;
 }
 else
 {
 newY = 0;
 moved = true;
 }
 break;
 case 3://North. If top reached try again
 if (newX > 0)
 {
 newX = newX - 1;
 moved = true;
 }
 break;
 case 4://South. If bottom reached try again
 if (newX < TableX - 1)
 {
 newX = newX + 1;
 moved = true;
 }
 break;
 default:
 break;
 }//switch
 }//if
 if (myArray[newX, newY].fishType == fishTypeEnum.fish)
 {
 Debug.WriteLine("Fisch gefangen");
 eatFish(newX, newY);
 myArray[x, y].ageStarvation = 0; ;
 }
 if (moved)
 {
 Debug.WriteLine("Hai bewegen");
 myArray[newX, newY].cellColor = myArray[x, y].cellColor;
 myArray[newX, newY].fishType = myArray[x, y].fishType;
 myArray[newX, newY].age = myArray[x, y].age + 1;
 myArray[newX, newY].ageStarvation = myArray[x, y].ageStarvation + 1;
 myArray[newX, newY].moved = true;
 //Shark will breed
 if (myArray[newX, newY].age >= ageToBreedShark)
 {
 myArray[newX, newY].age = 0;
 myArray[newX, newY].ageStarvation = 0;
 myArray[x, y].age = 0;
 myArray[x, y].ageStarvation = 0;
 myArray[x, y].moved = true;
 }
 else
 {
 kill(x, y); 
 }
 }
 }
 private void eatFish(int x, int y)
 {
 kill(x, y);
 }
 private void resetXY(ref int x, ref int y, int a,int b)
 {
 x = a;
 y = b;
 }
 //Searches for fish. howFar determines how many cells away sharks can find fish
 private int fishNearBy(int x, int y, int howFar)
 {
 int checkX = x;
 int checkY = y;
 int howClose =howFar+2;
 bool fishFound = false;
 int direction=0; //1=East, 2=West, 3=North, 4=South
 //Array for saving the direction. Distance initialized with howfar(maxlength)
 int[] directionArray = new int[5];
 for (int i = 0; i < directionArray.GetLength(0); i++)
 directionArray[i] = howFar + 1;
 //Check East
 for (int i = 0; i < howFar; i++)
 {
 if (checkY < 0)
 {
 checkY = TableY - 1;
 }
 if (myArray[x, checkY].fishType == fishTypeEnum.fish)
 {
 //Update distance
 directionArray[1] = i;
 fishFound = true;
 break;
 }
 checkY--;
 directionArray[1] = 99;
 }
 resetXY(ref checkX, ref checkY, x, y);
 //Check West
 for (int i = 0; i < howFar; i++)
 {
 if (checkY >= TableY)
 {
 checkY = 0;
 }
 if (myArray[x, checkY].fishType == fishTypeEnum.fish)
 {
 //Update distance
 directionArray[2] = i;
 fishFound = true;
 break;
 }
 checkY++;
 directionArray[2] = 99;
 }
 resetXY(ref checkX, ref checkY, x, y);
 //Check North
 for (int i = 0; i < howFar; i++)
 {
 if (checkX <= 0)
 {
 checkX = 0;
 }
 if (myArray[checkX, y].fishType == fishTypeEnum.fish)
 {
 //Update distance
 directionArray[3] = i;
 fishFound = true;
 break;
 }
 checkX--;
 directionArray[3] = 99;
 }
 resetXY(ref checkX, ref checkY, x, y);
 //Check South
 for (int i = 0; i < howFar; i++)
 {
 if (checkX >= TableX)
 {
 checkX = TableX-1;
 }
 if (myArray[checkX, y].fishType == fishTypeEnum.fish)
 {
 //Update distance
 directionArray[4] = i;
 fishFound = true;
 break;
 }
 checkX++;
 directionArray[4] = 99;
 }
 if (fishFound)
 {
 //Find the fish that is closest to the shark
 for (int i = 1; i < directionArray.GetLength(0); i++)
 {
 Debug.WriteLine("Fish found, Index: " + Array.IndexOf(directionArray, directionArray[i]) + " Entfernung: " + directionArray[i]);
 if (directionArray[i] < howClose)
 {
 Debug.WriteLine("Fish found, direction: " + Array.IndexOf(directionArray, directionArray[i]));
 howClose = directionArray[i];
 }
 direction = Array.IndexOf(directionArray, howClose);
 }
 }
 return direction;
 }
 private void move(int x, int y,fishTypeEnum type,bool repro)
 {
 int newX=x;
 int newY=y;
 int direction = 4;
 bool cellIsFree = false;
 bool noFreeCells = false;
 int numberOfTries = 0;
 int maxTries = 8;
 if ((type == fishTypeEnum.fish))
 {
 while (!cellIsFree && !noFreeCells)
 {
 direction = rnd.Next(1, 5);//Direction between 1-4 
 switch (direction)
 {
 case 1://East. If Index 0 move to TableEnde
 if (newY > 0)
 {
 newY = newY - 1;
 }
 else
 newY = TableY - 1;
 break;
 case 2://West. If TableEnde move to 0.
 if (newY < TableY - 1)
 {
 newY = newY + 1;
 }
 else
 newY = 0;
 break;
 case 3://North. If top reached try again
 if (newX > 0)
 {
 newX = newX - 1;
 }
 break;
 case 4://South. If bottom reached try again
 if (newX < TableX - 1)
 {
 newX = newX + 1;
 }
 break;
 }//switch
 if (myArray[newX, newY].fishType == fishTypeEnum.empty)
 {
 cellIsFree = true;
 }
 numberOfTries++;
 if (numberOfTries > maxTries)
 noFreeCells = true;
 }//if
 }//while (!cellIsFree)
 //Debug.WriteLine("NewX=" + newX + "NewY=" + newY);
 myArray[newX, newY].cellColor = myArray[x, y].cellColor;
 myArray[newX, newY].fishType = myArray[x, y].fishType;
 myArray[newX, newY].moved = true;
 myArray[newX, newY].age = myArray[x, y].age + 1;
 //If fish ist not reproducing remove the previous fish
 if (!repro)
 {
 kill(x, y);
 }
 else
 {
 myArray[newX, newY].age = 0;
 myArray[x, y].age = 0;
 myArray[x, y].moved = true;
 }
 }
 private void button2_Click(object sender, EventArgs e)
 {
 tick();
 Canvas.Refresh();
 }
 private void button1_Click(object sender, EventArgs e)
 {
 populate();
 Canvas.Refresh();
 }
}
}

The Cell class is used to store information about the cells:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Drawing;
namespace WaTor
{
//Fishtype
enum fishTypeEnum { fish, shark, empty };
class Cell
{
 //Color
 public Color cellColor { get; set; }
 //Age
 public int age {get;set;}
 //Age used for starvation
 public int ageStarvation { get; set; } 
 //Set&Get Fishtype
 public fishTypeEnum fishType {get;set;}
 //Check if moved during tick
 public bool moved { get; set; }
 //Position
 public int posX {get;set;}
 public int posY {get;set;}
 //Constructor
 public Cell()
 {
 cellColor = Color.Blue;
 posX = 0;
 posY = 0;
 age = 0;
 ageStarvation = 0;
 moved = false;
 fishType = fishTypeEnum.empty;
 }
}
}

• SuperBiasedMan: Welcome to Code Review! To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question.
• BCdotWEB: As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles.
• user3136418: Thanks for your feedback! the wator simulation ist what my code does, i mention that in the title.
• apieceoffruit: Ok, I'm a bit tired now but i'll give a full go at this later. In the short term the most obvious problem is highlighted by your comments. look at the populate function. why tell me what its doing. you know what would be better? Populate() { CleanTheGrid(); DistributeFish(); DistributeSharks(); } . read up on SRP (Single Responsibility Principle). same goes for the tick function. quick example: tick() { var currentCell = myArray[i,j]; if(IsShark(currentCell)) updateShark(); else updateFish(); }. try simple refactor into single units of work. then further OOP designs will appear

Answer 1

Well, first of all I would suggest you separate your concerns a little more. At the moment you have (most of) your Wa-Tor logic AND your Windows Forms code all together in the same class. Secondly, that class is named Form1! I know Visual Studio is nice when it does things for you, but naming classes isn't it's strong-suit ;)

Separate your presentation from your (domain) logic

As you get into more complex problem spaces, things like decomposition and domain modelling become important steps in translating the problem space into abstractions that you can compose together. Imagine if your Wa-Tor simulation was actually being run on a super computer, in order to produce an accurate model of sea-life populations and breeding patterns in the Pacific Ocean. This would obviously be a massive increase in scope, but shouldn't require any particularly large code changes* (known as refactoring) if your code has been written in a re-usable, decomposed, loosely-coupled way (which is the goal of all OOP coders).

^{* well, assuming the logic of Wa-Tor is exactly how the Ocean works.}

I would suggest that things such as initialNumberOfFish, initialNumberOfSharks, ageToBreedFish, ageToBreedShark & yearToStarvation, etc all belong somewhere other than the (presentation) class Form1. The same goes for the storage, size and initialization (intitializeArray) of your array (a better name for which might be "world", or ocean, etc). That class should probably be something along the lines of Simulation, World, etc.

The code in intitializeArray can go into the constructor of that Simulation class. That constructor can also accept all of the configuration parameters of the simulation (size, age to breed number of fish, etc) as arguments. It can also then call the populate method.

Also, an additional problem of your presentation code infecting your logical (model) code.. Why does a Cell have a cellColor? Colors are something pertaining to display, the Cell may have a state, status, type or even an isEmpty property - as these all relate to the simulation (which is what the Cell is part of modelling).

---

Abstract your behaviours as well as your representations...

- OR : "Don't repeat yourself!"

As @apieceoffruit commented, the Single Responsibility Principle (closely related to the Separation of Concerns I mentioned earlier) should be employed when you are deciding upon the responsibilities for your objects (often called "Don't Repeat Yourself", or DRY). Since objects contain both data (i.e. fields) and behaviour (methods), this principle should be used (as @apieceoffruit said) when designing your methods.

In particular, the populate method, contains the following code:

 //Distribute Fish randomly
 int PosX=0;
 int PosY=0;
 bool isEmpty=false;
 Random rndX = new Random();
 Random rndY = new Random();
 PosX = rndX.Next(0, TableX);
 for (int i = 0; i < initialNumberOfFish; i++)
 {
 while (!isEmpty)
 {
 PosX = rndX.Next(TableX);
 PosY = rndY.Next(TableY);
 if (myArray[PosX, PosY].fishType == fishTypeEnum.empty)
 isEmpty = true;
 }
 myArray[PosX, PosY].fishType = fishTypeEnum.fish;
 myArray[PosX, PosY].cellColor = Color.Green;
 myArray[PosX, PosY].age = rndX.Next(0, ageToBreedFish);
 isEmpty = false;
 }

Which is then almost copied verbatim for "Distribute Sharks randomly" (obviously with fishTypeEnum.shark used instead, etc).

This could (and would) be much better Decomposed into a helper method DistributeRandomly, or (as @apieceoffruit commented) the two methods: DistributeFish & DistributeSharks (hopefully which could share some common base helper method, since they're so similar).

There also seems to be something very similar in fishNearBy, which has four near identical for-loops. The may be able to be abstracted out into a helper method that fishNearBy can call 4 times, thus saving on code and making refactoring of that logic easier in the future.

---

Inheritance: (削除) The Answer to All your Problems (削除ここまで) (NOT)

As you mentioned in your question, "inheritance for fish and shark from the cell class", *might* be a good idea. It depends if your behaviours can be made generic enough to apply to both.

For example, in the Wikipedia page about Wa-tor^, it is stated:

Since hunting for fish takes priority over mere movement, the rules for a shark are more complicated: from the adjacent points occupied by fish, select one at random, move there and devour the fish. If no fish are in the neighborhood, the shark moves just as a fish does, avoiding its fellow sharks.

(Emphasis added)

^{^ Wikipedia attributes this (in a quotation) to Dewdney (1984) "Sharks and fish Wage an ecological War on the toroidal planet Wa-Tor", Scientific American December pg I4—22}

From reading this, I would have (as an OOP programmer naturally would,) thought that the move behaviour of a fish would therefore be something re-usable for a shark.

However, in your code it seems fish move while sharks hunt, and no re-use of move is made at all. This is something you may want to consider refactoring if you were to make Shark and Fish classes. For example, the current hunt method would become an overloaded move method on Shark. Shark would inherit from Fish (since a shark is a type of fish), and Fish would contain (a modified version of) the current move method. This way that code has been made re-usble (one of the goals of an OOP programmer).

---

Some General Code Smells

In addition to these, there are also just some general things I noticed going through you code, which as you say "is a mess" (well, a small mess anyway).

1. Multiple Random generators and instance member hiding. You class has Random rnd = new Random();, which is then repeated inside of hunt (not in itself a big issue, although I don't know why you need a different generator). But inside hunt, it is also named rnd, which means (as Visual Studio would underline it and say) that it ***hides* the instance member rnd* (in the class).

2. move takes a fishTypeEnum argument (named type, probably should be fishType to avoid confusion), and then uses this JUST to check that it is a fish. However, the *ONLY* place that move is called (in tick), this check is already done before move is called! The value fishTypeEnum.fish is also hard-coded into the call, making the if-statement to check the value inside move completely redundant.

3. This last one isn't really a 'coding' issue, but I'll include it for completeness. In your code, your comment for tick states that it represents a "round", however the Wikipedia page about Wa-tor says that these should be called "chronons".

---

Overall, I think your code is suffering from a lack of clarity of thought when designing. It has a "code first" feel, and as such the data structures and modelling are lacking. Some parts of it feel very procedural in style, other parts just seem more like spaghetti code (lacking a defined style).

I hope I've helped you with the things I've highlighted, comment if there's anything I've said that needs more explanation, or if you'd like me to go into more detail on anything.

• user3136418: just wow! I'll start redesigning the code asap
• user3136418: i hope a follow-up question is not against the rules: i seperated the presentation from the logic by creating a new Class Simulation, which contains all the world-parameters. 1. A simulation object is created from the WaTor (renamed Form1) Class. 2. From the simulation object, cell Objects are created. How can the cell objects access variables from the simulation instance that was created from the WaTor Class? My first thought would be to use a pointer to the object, but it doesnt seem to work.
• Tersosauros: Technically yes, but technically you're allowed to edit the question.. So technically, I don't think anybody minds? ;P As you say, from the Simulation object, Cell objects (technically called "instances," but let's not get too technical) are created. That part sounds good so far. My spidey-senses detect a C-programmer.. The Cell doesn't need to "access variables from Simulation" - these can be passed to the Cell(foo,bar) constructor.