patterncsharpMinor
Path finding - again
Viewed 0 times
againpathfinding
Problem
Nearly time to hand this project in, and I want it to be as close to perfect as possible. So, any issue (no matter how small) - let me know. If you have any ideas that would improve the efficiency or readability of my code (e.g. delegates), let me know.
I am already aware that I should use camelCase for variables and PascalCase for methods. I will do that at completion of the code.
```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Drawing;
namespace prjT02L08_Predator_Prey
{
public class AStar //Variant on Dijkstra's Algorithm - faster
{
public static Node[,] Grid; //Using the data type created below to make a grid, a 2 dimensional array of nodes (squares)
public static List FindPath(Point start, Point end) //Finds the fastest route from the start to end
{
//Checks that we aren't trying to make a path from one place to the same place
if (start.X == end.X && start.Y == end.Y)
{
return null;
}
//Resets all the parent variables to clear the paths created last time
for (int x = 0; x OpenList = new List(); //Nodes to be considered, ones that may be on the path
//The above OpenList would be better as an OrderedList, however, I wanted to implement my own sort
//An even better solution, and more "complex", would be to implement a Red-Black Tree
//I can get this to work, but am unsure about how to add a delete() method
//List ClosedList = new List();
//This is replaced by the dictionary below - Faster sorting using a key
//Dictionary ClosedList = new Dictionary();
//This is replaced by the HashSet below - O(1) instead of O(n) for searching and removing data.
//Also O(1) for adding, unless the size needs to be increased (then O(n))
HashSet ClosedList = new HashSet(); //Explored nodes
Point CurrentPoint = new Point(0, 0);
Node Cu
I am already aware that I should use camelCase for variables and PascalCase for methods. I will do that at completion of the code.
AStar class:```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Drawing;
namespace prjT02L08_Predator_Prey
{
public class AStar //Variant on Dijkstra's Algorithm - faster
{
public static Node[,] Grid; //Using the data type created below to make a grid, a 2 dimensional array of nodes (squares)
public static List FindPath(Point start, Point end) //Finds the fastest route from the start to end
{
//Checks that we aren't trying to make a path from one place to the same place
if (start.X == end.X && start.Y == end.Y)
{
return null;
}
//Resets all the parent variables to clear the paths created last time
for (int x = 0; x OpenList = new List(); //Nodes to be considered, ones that may be on the path
//The above OpenList would be better as an OrderedList, however, I wanted to implement my own sort
//An even better solution, and more "complex", would be to implement a Red-Black Tree
//I can get this to work, but am unsure about how to add a delete() method
//List ClosedList = new List();
//This is replaced by the dictionary below - Faster sorting using a key
//Dictionary ClosedList = new Dictionary();
//This is replaced by the HashSet below - O(1) instead of O(n) for searching and removing data.
//Also O(1) for adding, unless the size needs to be increased (then O(n))
HashSet ClosedList = new HashSet(); //Explored nodes
Point CurrentPoint = new Point(0, 0);
Node Cu
Solution
-
It is better to declare by interface instead of implementation.
Can become:
This allows you to more easily change the implementation without having to change on multiple spaces. (This is extra important when passing objects between methods!)
-
Initialize
-
3 10 is a magic number, yes it is, it's a magic number. If you would like to change the move cost on the map you would have to modify at multiple places, modifying some but not all of the occurrences would lead to unexpected behavior (bugs!). Declare a
-
I am not so sure about that ternary statement and the condition
-
In your
-
Split the
-
Overall, your code is well formatted and very easy to read. You use quite good variable and method names. It has been a pleasure reading your code. You seem to know what you are doing and I hope you will continue on your path. (Where-ever the
It is better to declare by interface instead of implementation.
List OpenList = new List();
HashSet ClosedList = new HashSet();Can become:
IList OpenList = new List();
ISet ClosedList = new HashSet();This allows you to more easily change the implementation without having to change on multiple spaces. (This is extra important when passing objects between methods!)
-
Initialize
CurrentPoint to null instead of creating a new point at (0, 0). That point is currently not used.-
3 10 is a magic number, yes it is, it's a magic number. If you would like to change the move cost on the map you would have to modify at multiple places, modifying some but not all of the occurrences would lead to unexpected behavior (bugs!). Declare a
const int MoveCost = 10; and use that throughout your code.-
I am not so sure about that ternary statement and the condition
OpenList.Count > 1. It's been a while since I used A* but I believe it's possible for all nodes to be added to ClosedList and thus the entire map has been exhausted before finally a path was found (consider a map where there is only one possible path to go, entirely surrounded by walls). I would use a different condition here, like Path.Count > 1.-
In your
GetNeighbours method you can use the yield keyword and use the return type IEnumerable to avoid having to initialize a list.-
Split the
FindPath method at least in two, one for reaching the goal and one for backtracking.-
Overall, your code is well formatted and very easy to read. You use quite good variable and method names. It has been a pleasure reading your code. You seem to know what you are doing and I hope you will continue on your path. (Where-ever the
ClosedList leads you)Code Snippets
List<Node> OpenList = new List<Node>();
HashSet<Node> ClosedList = new HashSet<Node>();IList<Node> OpenList = new List<Node>();
ISet<Node> ClosedList = new HashSet<Node>();Context
StackExchange Code Review Q#30606, answer score: 5
Revisions (0)
No revisions yet.