6
\$\begingroup\$

I've made this relatively basic height map generator on my own just to see if I could pull it off because I'm still a newbie when it comes to programming. So my question is: "Is there a fairly obvious way I can optimize it?". I put in effort to make the code as clean as I can write it so any input from a quick review (if you have nothing better to do of course) would be greatly appreciated. Here is the ORIGINAL code:

using System;
using System.Diagnostics;
namespace height_map_generator
{
 class Program
 {
 private static readonly int bufferWidth = 237;
 private static readonly int bufferHeight = 90;
 private static readonly double nodeProbability = 0.007; //Initial node spawn probability in each point of buffer array. (The higher this number is the more peaks on the map.)
 private static readonly double intervalFraction = 0.03; //Number must be between 0 and 1. (The lower this number - the higher the variability and sharpness of terrain.)
 private static readonly double maxDistanceFromCentre = Math.Sqrt(Math.Pow(bufferWidth / 2, 2) + Math.Pow(bufferHeight / 2, 2));
 private static readonly double distanceInterval = maxDistanceFromCentre * intervalFraction;
 private static readonly Random rand = new Random();
 private static int[,] mapBuffer = new int[bufferWidth, bufferHeight];
 private static int[,] nodePositionValue = new int[bufferWidth, bufferHeight];
 private static int[] nodeXposition = new int[bufferWidth * bufferHeight];
 private static int[] nodeYposition = new int[bufferHeight * bufferWidth];
 private static int nodeCount = 0; //Currently not in use.
 static void Main(string[] args)
 {
 Console.SetBufferSize(bufferWidth, bufferHeight);
 Console.ForegroundColor = ConsoleColor.Green;
 string elapsedTime = null;
 Stopwatch sw = new Stopwatch();
 sw.Start();
 constructNodes(nodeProbability); // First.
 setNodeCoordinates(); // Second.
 compareDistancesAndSetEachPoint(); // Third.
 sw.Stop();
 elapsedTime = sw.Elapsed.ToString();
 writeTerrain(); // Final.
 Console.SetBufferSize(bufferWidth, bufferHeight + 2);
 Console.WriteLine();
 Console.Write("Operation completed in: " + elapsedTime);
 Console.ReadKey(); //Wait before quitting.
 }
 private static void constructNodes(double probability)
 {
 for (int y = 0; y < bufferHeight; y++)
 {
 for (int x = 0; x < bufferWidth; x++)
 {
 if (probabilityResult(probability))
 {
 int nodeAmp = generateNodeAmplitude();
 mapBuffer[x, y] = nodeAmp;
 nodePositionValue[x, y] = nodeAmp;
 }
 }
 }
 }
 private static void setNodeCoordinates()
 {
 int nodes = 0;
 for (int y = 0; y < bufferHeight; y++)
 {
 for (int x = 0; x < bufferWidth; x++)
 {
 if (nodePositionValue[x, y] != 0)
 {
 nodeXposition[nodes] = x;
 nodeYposition[nodes] = y;
 nodes++;
 }
 }
 } 
 nodeCount = nodes;
 }
 private static void compareDistancesAndSetEachPoint()
 {
 for (int y = 0; y < bufferHeight; y++)
 {
 for (int x = 0; x < bufferWidth; x++)
 {
 if (spaceAvailable(x, y))
 {
 int n = 0;
 double minimumDistance = 0;
 int nodeAmplitude = 0;
 foreach (int i in nodeXposition)
 {
 if (n == 0)
 minimumDistance = Math.Sqrt(Math.Pow(x - nodeXposition[n], 2) + Math.Pow(y - nodeYposition[n], 2));
 else if (minimumDistance > Math.Sqrt(Math.Pow(x - nodeXposition[n], 2) + Math.Pow(y - nodeYposition[n], 2)))
 {
 minimumDistance = Math.Sqrt(Math.Pow(x - nodeXposition[n], 2) + Math.Pow(y - nodeYposition[n], 2));
 nodeAmplitude = nodePositionValue[nodeXposition[n], nodeYposition[n]];
 }
 n++;
 }
 setAmplitude(x, y, minimumDistance, nodeAmplitude);
 }
 }
 } 
 }
 private static void writeTerrain()
 {
 for (int y = 0; y < bufferHeight; y++)
 {
 for (int x = 0; x < bufferWidth; x++)
 {
 Console.SetCursorPosition(x, y);
 Console.Write(mapBuffer[x, y]);
 }
 }
 }
 /*******************************************************************************/
 private static void setAmplitude(int x, int y, double distance, int nodeAmplitude)
 {
 int k = 1;
 while (true)
 {
 if (distance <= k * distanceInterval)
 {
 //---------------------------------------COSMETIC 'IF' STATEMENT----------------------------------------- (Should be removed when using the data.)
 if (k + nodeAmplitude <= 9)
 mapBuffer.SetValue(nodeAmplitude + k, x, y);
 else
 mapBuffer.SetValue(9, x, y);
 //-------------------------------------------------------------------------------------------------------
 break;
 }
 k++;
 }
 }
 private static bool spaceAvailable(int x, int y)
 {
 if (nodePositionValue[x, y] == 0)
 return true;
 else
 return false;
 }
 private static bool probabilityResult(double percent)
 {
 if (percent <= 0 || percent > 1)
 throw new ArgumentOutOfRangeException("Probability must be between 0 and 1.");
 return rand.NextDouble() <= percent;
 }
 private static int generateNodeAmplitude()
 {
 int num = rand.Next(1, 4);
 return num;
 }
 }
}
asked Apr 19, 2017 at 17:08
\$\endgroup\$
16
  • \$\begingroup\$ It's kind of hard to improve the code with very random naming. The meaning of things like space or nodePosVal is hard to infer even by looking at the usage examples. Here're a few broad suggestions: spell things out -- don't be lazy; find a meaningful name for a given entity; use expressive names for methods (void getNodeCount() is confusing, we call get but void doesn't return anything back)... \$\endgroup\$ Commented Apr 19, 2017 at 17:27
  • \$\begingroup\$ 'getNodeCount' used to return the total node count in the 2d array but I later modified it to just set a variable as a void function and forgot to edit it's name. I'll edit the post ASAP. \$\endgroup\$ Commented Apr 19, 2017 at 17:31
  • \$\begingroup\$ I also see the object-oriented tag, and nothing suggests objects are used in here to model maps \$\endgroup\$ Commented Apr 19, 2017 at 17:31
  • 2
    \$\begingroup\$ They are not cosmetic, when you rename methods, change variable scope, change loop constructs, and so on. These kind of changes invalidates answers, discourages reviewers, and you're actually ruining your chances for us to give you good reviews. \$\endgroup\$ Commented Apr 19, 2017 at 18:48
  • 2
    \$\begingroup\$ I understand. I am truly sorry for my ignorance on the matter. I am new to CodeReview so I didn't really think about the confusion I was causing when changing the code. I will never do this again. I am really sorry for any inconvenience caused. \$\endgroup\$ Commented Apr 19, 2017 at 18:52

5 Answers 5

5
\$\begingroup\$

A couple quick things:

If you are using while loops for iterations, just use for loops.

//I have to look in three places to understand how the loop works
int x = 0;
while(x < 10){x++;}
//Everything I need is right here 
for(int x = 0; x < 10; x++){}

In probability result, you can drop the if and the new variable. Just do it like this

private static bool probabilityResult(double percent)
{
 if (percent <= 0 || percent > 1)
 throw new ArgumentOutOfRangeException("Probability must be between 0 and 1.");
 return rand.NextDouble() <= percent;
}

You can apply the same logic to several of your other functions. No need to save data to temp var if you're going to return it immediately

As far as actual performance goes, instead of using a multidimensional array ([,]), use a jagged array([][]). Due to compiler optimizations jagged arrays often perform better.

answered Apr 19, 2017 at 18:04
\$\endgroup\$
2
  • \$\begingroup\$ Thank you very, very much for the awesome feedback dude. I will try to integrate your suggestions. I'll have to read up on jagged arrays. Anyway, thanks so much and have an awesome day! :) \$\endgroup\$ Commented Apr 19, 2017 at 18:07
  • \$\begingroup\$ I wanted to update you on the situation Zebraman. Just to let you know I replaced the arrays with jagged arrays and the code takes about 3 seconds longer to execute. I suspect it's because of the needed initialization of the jagged arrays in the beginning but I'm not quite sure. \$\endgroup\$ Commented Apr 19, 2017 at 19:44
4
\$\begingroup\$

DRY

for (int y = 0; y < bufferHeight; y++)
{
 for (int x = 0; x < bufferWidth; x++)
 {
 }
}

If you happen to write the same code more then once it usually means something needs to be encapsulated.

As far as loops are concerned you have two choices:

  1. convenience first - go with LINQ and helper strucutres like Point
  2. performance first - go with loops and delegates

I prefer the first option and switch do the second one only if something really seems to run slow and I notice and measured it.

So how would it look like if you ware lazy and wanted to get the job done quickly? You would create a map point generator:

public static class Map
{ 
 public static IEnumerable<Point> GeneratePoints(int width, int height)
 {
 for (int y = 0; y < height; y++)
 for (int x = 0; x < width; x++)
 yield return new Point(x, y);
 }
}

that you can reuse everywhere you need the coordinates:

foreach(var point in Map.GeneratePointS(bufferWidth, bufferHeight))
{
 ...
}

Assuming the spaceAvailable method works with Point you could greatly shorten the compareDistancesAndSetEachPoint to soemthing like this:

var availablePoints = Map.GeneratePoints(bufferWidth, bufferHeight).Where(spaceAvailable);
foreach (var point in availablePoints)
{
 ...
}

In the second case you would use a delegate:

public static void ForEachPoint(int width, int height, Action<int, int, int> action)
{
 var i = 0;
 for (int y = 0; y < height; y++)
 for (int x = 0; x < width; x++)
 action(x, y, i++);
}

so the processing would now look like this

Map.ForEachPoint(bufferWidth, bufferHeight), (x, y, i) =>
{
 ...
});

where for each point the action is executed however its signature Action<int, int, int> isn't easy to understand and actually needs commenting. You can avoid this by creating a custom delegate:

public delegate void ProcessPointCallback(int x, int y, int index);

you would then change the signature of the ForEachPoint method to

public static void ForEachPoint(int width, int height, ProcessPointCallback processPoint) 

and the rest remains the same.


Other suggestions

private static bool spaceAvailable(int x, int y)
{
 if (nodePositionValue[x, y] == 0)
 return true;
 else
 return false;
}

You don't need an if for something like this. It's better to just write:

private static bool spaceAvailable(int x, int y)
{
 return nodePositionValue[x, y] == 0;
}

while (true)
{
 if (distance <= k * distanceInterval)
 {
 ...
 }
 k++;
}

You should try to avoid nesting and actually use the while() as an if. This means you should put the condition for the loop where it belongs (and adjusting it of course):

while (distance > k * distanceInterval)
{
 ...
}
answered Apr 21, 2017 at 7:36
\$\endgroup\$
2
  • \$\begingroup\$ Wow. Such an informative review. I am really thankful for the points you made as I too was feeling that I shouldn't be repeating myself so much. The more object-oriented approach is preferable and I understand that because my code becomes really rigid and that's always a bad thing. The problem with me is that I seem to have this urge to always just do something. I haven't even begun to grasp the syntax and possibilities of C# and OOP but I just wanted to see what I could pull off with really bare bones basic knowledge. I have yet to learn all about delegates and the like and I will do so. \$\endgroup\$ Commented Apr 21, 2017 at 19:36
  • \$\begingroup\$ Sadly I still have important exams at the end of my last year in high school which I have to tent to first and I will for sure dedicate my entire summer to learn as much of the theory behind the language as I can. And again... thank you very much for dedicating your time for this review! :) \$\endgroup\$ Commented Apr 21, 2017 at 19:39
3
\$\begingroup\$

Ideally, you should not orchestrate the entire lifecycle of your application from Program class, but I left it...

There are also lots of things I was unable to understand, and I left them aside [consider as a "homework" for you :)].

I am using a few classes since I believe you wanted to see a more object oriented approach.

Notice that the code is still neither super clean (OOP-wise), nor super efficient, but gives you the basic direction (@Zebraman has a great comment on the underlying data structure you use). Let me know if you want anything to be explained in detail.

class Program
{
 private static readonly int bufferWidth = 237;
 private static readonly int bufferHeight = 90;
 private static readonly double nodeSpawnProbability = 0.007; //Initial node spawn probability in each point of buffer array. (The higher this number is the more peaks on the map.)
 private static readonly double intervalFraction = 0.03; //Number must be between 0 and 1. (The lower this number - the higher the variability and sharpness of terrain.)
 private static readonly double maxDistanceFromCentre = Math.Sqrt(Math.Pow(bufferWidth / 2, 2) + Math.Pow(bufferHeight / 2, 2));
 private static readonly double distanceInterval = maxDistanceFromCentre * intervalFraction;
 static void Main(string[] args)
 {
 Console.SetBufferSize(bufferWidth, bufferHeight);
 Console.ForegroundColor = ConsoleColor.Green;
 var stopwatch = new Stopwatch();
 stopwatch.Start();
 var grid = new GridFactory()
 .CreateGrid(bufferWidth, bufferHeight, nodeSpawnProbability); // First.
 // Variable is unused...
 var nodeCount = grid.NodeCount; // Second.
 // CompareAndSet(grid); // Third.
 stopwatch.Stop();
 WriteTerrain(grid); // Final.
 Console.SetBufferSize(bufferWidth, bufferHeight + 2);
 Console.WriteLine();
 Console.Write("Operation completed in: " + stopwatch.Elapsed.ToString());
 Console.ReadKey(); //Wait before quitting.
 }
 // private static void CompareAndSet(Grid grid)
 // {
 // // TODO
 // }
 private static void WriteTerrain(Grid gridToPlot)
 {
 foreach (var location in gridToPlot.AllLocations)
 {
 Console.SetCursorPosition(location.X, location.Y);
 Console.Write(gridToPlot[location.X, location.Y]);
 }
 }
 private static void UpdateGridAmplitudes(Grid grid, int x, int y, double distance, int threshold)
 {
 int k = 1;
 while (true)
 {
 if (distance <= k * distanceInterval)
 {
 if (k + threshold <= 9)
 grid[x, y].Amplitude = threshold + k;
 else
 grid[x, y].Amplitude = 9;
 break;
 }
 k++;
 }
 }
}

Something that creates a Grid:

public class GridFactory
{
 private static readonly Random random = new Random();
 public Grid CreateGrid(int bufferWidth, int bufferHeight, double nodeSpawnProbability)
 {
 var grid = new Grid(bufferWidth, bufferHeight);
 foreach (var location in grid.AllLocations)
 {
 if (TrueIfRandomWith(nodeSpawnProbability))
 grid[location.X, location.Y].Amplitude = RandomAmplitude();
 }
 return grid;
 }
 private bool TrueIfRandomWith(double probability)
 {
 if (probability <= 0 || probability > 1)
 throw new ArgumentOutOfRangeException("Probability must be between 0 and 1.");
 return random.NextDouble() <= probability;
 }
 private int RandomAmplitude()
 {
 return random.Next(1, 4);
 }
}

Grid that represents a map:

public class Grid
{
 private readonly int _bufferWidth;
 private readonly int _bufferHeight;
 private readonly GridCell[,] _matrix;
 public Grid(int bufferWidth, int bufferHeight)
 {
 _bufferWidth = bufferWidth;
 _bufferHeight = bufferHeight;
 _matrix = new GridCell[_bufferWidth, _bufferHeight];
 foreach (var location in AllLocations)
 _matrix[location.X, location.Y] = new GridCell();
 }
 public GridCell this[int x, int y]
 {
 get
 {
 return _matrix[x, y];
 }
 set
 {
 _matrix[x, y] = value;
 }
 }
 public IEnumerable<GridLocation> AllLocations
 {
 get
 {
 var xs = Enumerable.Range(0, _bufferWidth);
 var ys = Enumerable.Range(0, _bufferHeight);
 var allGridCoordinates = xs.SelectMany(x => ys.Select(y => new GridLocation { X = x, Y = y }));
 return allGridCoordinates;
 }
 }
 public int NodeCount
 {
 get
 {
 return AllLocations
 .Count(location => this[location.X, location.Y].IsAvailable == false);
 }
 }
}

Grid location ("coordinate")

public class GridLocation
{
 public int X { get; set; }
 public int Y { get; set; }
}

Grid cell:

public class GridCell
{
 public bool IsAvailable { get; set; } = true;
 public int Amplitude { get; set; } = 0;
}

Update 1

There tons and tons of materials that explain what are the strengths (and the weaknesses) of the OOP. You can start from here.

Here are a few ideas about the application of OOP principles.

In OOP-ed code, the data and the related processing functions (methods) are encapsulated into small entities (classes). The encapsulation has various positive effects, and a few negative effects.

You may see that with the rewritten code we can change Grid, GridLocation, and GridCell more independently. For example, if we need to have some extra information about each location on the map, we may add some fields to GridCell class rather than creating a whole new int[,] extraDataMap which is by the way completely disconnected from the space and other maps...

It's really hard to explain the benefits of OOP in one example, but I think more will come as you learn through reading and practicing.

answered Apr 19, 2017 at 18:27
\$\endgroup\$
4
  • \$\begingroup\$ Please try to reason why you consider this to be a better solution than the OP's own solution. Or what kind of process lead to this solution. Remember that this is Code Review, not simply Code Rewrite. \$\endgroup\$ Commented Apr 19, 2017 at 18:35
  • \$\begingroup\$ This is proficiently tied together and it will definitely help me integrate OOP better in the future. I am very thankful for the effort you put in to this. I'll have more time to study this tomorrow but I have one initial question - What is the underscore's purpose when naming a variable or method alike? \$\endgroup\$ Commented Apr 19, 2017 at 18:37
  • \$\begingroup\$ @SimeonLaplev I will update the post with some references to OOP-related materials and discussions. The underscore is sometimes used to avoid name collision (e.g. bufferWidth = bufferWidth will reassign a parameter value to itself; we can either do this.bufferWidth = bufferWidth; or just rename the field we want to assign the value to). Many people do not like private _fieldName naming convention, but it's a matter of preference. I am neither encouraging nor discouraging from using such convention. \$\endgroup\$ Commented Apr 19, 2017 at 18:41
  • \$\begingroup\$ Aha... I see. So the naming convention is rather trivial as I understand. Ok. Thanks for the heads up and I will keep that in mind from now on. \$\endgroup\$ Commented Apr 19, 2017 at 18:44
2
\$\begingroup\$

The thing that screams "BEGINNER!" is the fact that you've placed everything in the Program class. Igor's opening remark touches upon this, but he doesn't do anything beyond giving it a mention. My remarks below may be used in conjunction with all the other answers.

As written, your code is extremely rigid. What if you ever need a different height or width? Or node probability or interval fraction? Those 4 properties seem to define a very specific grid. This is achieved by creating a class with an appropriate name, perhaps Grid or GridMap or something else as long as it is a meaningful, clear name.

You have 4 main properties (previously mentioned) that help describe the uniqueness of one grid over another. At a minimum, you would have a class defined as something like:

public class GridMap
{
 public int Width { get; }
 public int Height { get; }
 public double NodeProbability { get; }
 public double IntervalFraction { get; }
 public GridMap(int bufferWidth, int bufferHeight, double nodeProbability, double intervalFraction)
 {
 //I leave it as homework that you validate the inputs
 Width = bufferWidth;
 Height = bufferHeight;
 NodeProbability = nodeProbability;
 IntervalFraction = intervalFraction; 
 }
}

That is our launching point. Let's build from there. We will add a few more constructors.

You have a few spots where you use Magic Numbers (example: see generateNodeAmplitude), but you also have spots where you define some static readonly fields. As these are value type fields, you could make these constants.

public class GridMap
{
 public int Width { get; }
 public int Height { get; }
 public double NodeProbability { get; }
 public double IntervalFraction { get; }
 public const int DefaultWidth = 237;
 public const int DefaultHeight = 90;
 public const double DefaultNodeProbability = 0.007; 
 public const double DefaultIntervalFraction = 0.03;
 public GridMap(int bufferWidth, int bufferHeight, double nodeProbability, double intervalFraction)
 {
 //I leave it as homework that you validate the inputs
 Width = bufferWidth;
 Height = bufferHeight;
 NodeProbability = nodeProbability;
 IntervalFraction = intervalFraction; 
 }
 public GridMap(int bufferWidth, int bufferHeight) : this(bufferWidth, bufferHeight, DefaultNodeProbability, DefaultIntervalFraction)
 { }
 public static GridMap DefaultGrid()
 {
 return new GridMap(DefaultWidth, DefaultHeight, DefaultNodeProbability, DefaultIntervalFraction);
 }
}

Now you've opened up a world of possibilities for a much larger combinations of grids! Of course, many of your static methods in the Program class need to be moved into this new class, and no longer be static. That's left as homework for you.

answered Apr 21, 2017 at 15:07
\$\endgroup\$
1
  • \$\begingroup\$ I value the criticism about the rigidity of my code and will work upon my methods in the future. I am truly a beginner and since I don't have the needed experience I don't think about convenience when I'm writing the program I just think about the next step and that's all I have in my mind. Those logical conjunctions in my head I translate to code in basic 'if' statements and 'loops' without perspective, one after another. Fortunately, I recognize this and I assure you I will try my best in the future to do it correctly. At the moment school is a priority though. Thanks again, dude! \$\endgroup\$ Commented Apr 21, 2017 at 19:47
1
\$\begingroup\$

I have revised my program today and used some of the tips I got from the answers on this post. I went towards a more object-oriented approach and completely rewrote the program. I saw a MASSIVE performance increase. From 1:05 minutes to about 11 seconds! I have used 3 class files total. (counting the initial Program.cs class file) I will now post the code for the revised program here for future reference.

Program.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Diagnostics;
using System.Threading.Tasks;
namespace testing_program
{
 class Program : Map
 {
 private static readonly int bufferWidth = 237;
 private static readonly int bufferHeight = 90;
 private static readonly double intervalFraction = 0.02;
 private static readonly double maxDistanceFromCentre = Math.Sqrt(Math.Pow(bufferWidth / 2, 2) + Math.Pow(bufferHeight / 2, 2));
 private static readonly double distanceInterval = maxDistanceFromCentre * intervalFraction;
 static void Main(string[] args)
 {
 Console.SetBufferSize(bufferWidth, bufferHeight);
 Console.ForegroundColor = ConsoleColor.Green;
 string elapsedTime = null;
 Stopwatch sw = new Stopwatch();
 sw.Start();
 GeneratePoints(bufferWidth, bufferHeight); //GeneratePoints is a method from the Map class.
 compareAndSetConnectedNodes();
 setPointsAmplitude();
 sw.Stop();
 elapsedTime = sw.Elapsed.ToString();
 writeTerrain();
 Console.SetBufferSize(bufferWidth, bufferHeight + 2);
 Console.WriteLine();
 Console.Write("Operation completed in: " + elapsedTime);
 Console.ReadKey(); 
 }
 private static void writeTerrain()
 {
 foreach (Point point in points)
 {
 Console.SetCursorPosition(point.x, point.y);
 Console.Write(point.Amplitude);
 }
 }
 private static void compareAndSetConnectedNodes()
 {
 foreach (Point point in points) //For reference - 'points' is a list of all points generated by the Map class (points is a member of the Map class)
 {
 if (!point.IsNode)
 {
 int n = 0;
 double minimumDistance = n;
 foreach (Point p in points)
 {
 if (p.IsNode)
 {
 if (n == 0)
 {
 minimumDistance = Math.Sqrt(Math.Pow(point.x - p.x, 2) + Math.Pow(point.y - p.y, 2));
 point.DistanceToNode = minimumDistance;
 }
 else if (minimumDistance > Math.Sqrt(Math.Pow(point.x - p.x, 2) + Math.Pow(point.y - p.y, 2)))
 {
 minimumDistance = Math.Sqrt(Math.Pow(point.x - p.x, 2) + Math.Pow(point.y - p.y, 2));
 point.DistanceToNode = minimumDistance;
 }
 n++;
 }
 }
 }
 }
 }
 private static void setPointsAmplitude()
 {
 foreach (Point point in points)
 {
 if (!point.IsNode)
 {
 int k = 1;
 while (point.DistanceToNode > k * distanceInterval)
 {
 k++;
 }
 //---------------------------------------COSMETIC 'IF' STATEMENT----------------------------------------- (Should be removed when using the data.)
 if (k + point.ConnectedNodeAmplitude <= 9)
 point.Amplitude = point.ConnectedNodeAmplitude + k;
 else
 point.Amplitude = 9;
 //-------------------------------------------------------------------------------------------------------
 }
 }
 }
 }
}

Map.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace testing_program
{
 public class Map
 {
 protected static List<Point> points = new List<Point>();
 public static void GeneratePoints(int width, int height)
 {
 for (int y = 0; y < height; y++)
 for (int x = 0; x < width; x++)
 points.Add(new Point(x, y));
 }
 }
}

Point.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
namespace testing_program
{
 public class Point
 {
 private static double nodeProbability = 0.01;
 private static int lowestNodeValue = 3;
 private bool _isNode;
 private int _x;
 private int _y;
 private double distanceToNode = 0;
 private int amplitude = 0;
 private int connectedNodeAmplitude;
 private static Random randy = new Random(); //randy is a cool name for a Random() object :D
 public Point(int x, int y)
 {
 _x = x;
 _y = y;
 if (isNode(nodeProbability))
 {
 _isNode = true;
 amplitude = randy.Next(1, lowestNodeValue + 1);
 }
 }
 private static bool isNode(double nodeProbability)
 {
 return randy.NextDouble() <= nodeProbability; 
 }
 //Property declarations---------------------------------------------------
 public int x
 {
 get { return _x; }
 }
 public int y
 {
 get { return _y; }
 }
 public double NodeProbability
 {
 get { return nodeProbability; }
 set { if (value > 0 && value <= 1) nodeProbability = value; }
 }
 public int LowestNodeValue
 {
 get { return lowestNodeValue; }
 set { if (value > 0) lowestNodeValue = value; }
 }
 public int ConnectedNodeAmplitude
 {
 get { return connectedNodeAmplitude; }
 set { if (value > 0) connectedNodeAmplitude = value; }
 }
 public int Amplitude
 {
 get { return amplitude; }
 set { if (value > 0) amplitude = value; }
 }
 public double DistanceToNode
 {
 get { return distanceToNode; }
 set { distanceToNode = value; }
 }
 public bool IsNode
 {
 get { return _isNode; }
 }
 //------------------------------------------------------------------------
 }
}

NOTE: I am sorry if the code isn't absolutely touched up or is lacking comments at most places. This is all I had time to do this Sunday. Thank you to EVERYONE that participated in this thread and helped me view my mistakes and take on a better perspective on the situation. Really. You all rock!

answered Apr 23, 2017 at 17:12
\$\endgroup\$
6
  • \$\begingroup\$ Perfect! I'm glad you were able to improve your code ;-) \$\endgroup\$ Commented Apr 23, 2017 at 17:14
  • 1
    \$\begingroup\$ Same. It felt awesome working with more classes and types and properties. I felt like a kid with Lego bricks. :D The performance increase honestly shook me. Who new getting rid of all those arrays would result in a performance boost. ;) \$\endgroup\$ Commented Apr 23, 2017 at 17:16
  • \$\begingroup\$ By the way, may I quickly ask if it was a good idea to use a 'List' to store the Points? If not, what else can I use in it's place? I'm not sure if it fits the purpose correctly. \$\endgroup\$ Commented Apr 23, 2017 at 20:19
  • \$\begingroup\$ I find a List is fine and most probably I'd use it myself... or an array but there are not many alternatives. One could say if the number of points is constant an array might be more appropriate. As a matter of fact if the list never changes currently I'd use the IImmutableArray from the System.Collections.Immutable NuGet package. \$\endgroup\$ Commented Apr 23, 2017 at 20:25
  • \$\begingroup\$ I see. Thanks for the response. I'll make sure to look up 'IImmutableArray'. But if I use an array how do I specify it's length? Just make it 'height * width' long by calling a constructor to set it? \$\endgroup\$ Commented Apr 23, 2017 at 20:35

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.