A* algorithm for hex tiles

I implemented the A* algorithm for pathfinding for a game of mine. The game uses hex rooms which are connected to each other by an int[] in the room class. Are there any pointers you can give me on my code?

```
public class Pathfinding
{
///
/// Finds a path from one room to another using the given algorithm, or A* by default. Returns null if a path is not found.
///
/// The starting point
/// The end point
/// The algorithm to find a path with
/// A list of rooms making a path, or null if there is not path.
public static List FindPath(Room start, Room end, Cave cave, PathfindingAlgorithm algorithm = PathfindingAlgorithm.A_STAR)
{
switch (algorithm)
{
case PathfindingAlgorithm.A_STAR: return FindAStarPath(start, end, cave);

default: throw new Exception();
}
}

private static List FindAStarPath(Room start, Room end, Cave cave)
{
int MAX_TRAVERSED_ROOMS = cave.getRoomDict().Count;
IPriorityQueue> openNodes = new HeapPriorityQueue>(MAX_TRAVERSED_ROOMS);
List> closedNodes = new List>();

Func, IEnumerable>> getNeighbors = (r) =>
{
return new List(r.node.adjacentRooms).Select((a) => new AStarNode(cave.GetRoom(a), r));
};
// Add the start node with an F cost of 0
openNodes.Enqueue(new AStarNode(start), 0);

while (openNodes.Count != 0)
{
//The one with the least F cost
AStarNode current = openNodes.Dequeue();
closedNodes.Add(current);

foreach (AStarNode neighbor in getNeighbors(current))
{
// if we already processed this node
if (closedNodes.Contains>(neighbor)) continue;

int fCost = GetEstimatedScore(neighbor.node, end, cave) + nei

Your enum+switch design to choose which algorithm to apply is very procedural. In an object-oriented world, it is much more common to see:

public interface IPathfinder {
    List FindPath(Room start, Room end, Cave cave);
}

public class AStarPathfinder : IPathfinder {
    public List FindPath(Room start, Room end, Cave cave) {
        // ...
    }
}

And consider changing the return type List to the more generic IList. It is usually considered best practice to expose interfaces instead of concrete implementations. Check this StackOverflow discussion.

Your lambda function is cluttering the visual space of your algorithm implementation, I would rather put it in a separate private method. In this function, r is not a meaningful name: even anonymous functions should have meaningful names for its parameters. Room.adjacentRooms is of type int[], and it implements IEnumerable, so no need to instantiate a new List.

private IEnumerable> GetNeighbors(AStarNode roomNode, Cave cave) {
    return roomNode.node.adjacentRooms
        .Select(roomIndex => new AStarNode<Room(cave.GetRoom(roomIndex), roomNode));
}

Similar to the IPathfinder interface, you could not restrain yourself to using only one heuristic (GetEstimatedScore). You could set a general interface for heuristics

public interface IHeuristic {
     int Estimate(Room start, Room end, Cave cave);
 }

 public class ManhattanDistanceHeuristic : IHeuristic {
     public int Estimate(Room start, Room end, Cave cave) {
        Vector2 startPos = cave.RoomLayout[start.roomId].RoomPosition;
        Vector2 endPos = cave.RoomLayout[end.roomId].RoomPosition;
        return (int)Math.Round(Math.Abs(startPos.X - endPos.X) + Math.Abs(startPos.Y - endPos.Y));
    }
 }

And of course, add a IHeuristic property to your AStarPathfinder class, so you can inject this dependency later.

Your class AStarNode is implicitly declared as internal visibility, but I suppose only the AStarPathfinder class would use it, so it could be a private class inside AStarPathfinder. Furthermore, while the generics is cool, I also suppose it is only ever getting instanced as AStarNode, so you could've kept it simpler.

Your Room class has room (ha.) for some improvement. First, it has public read-write fields, including an array. You did fine in the visibility for the other classes, but here it is thrown away. Furthermore, your gold and arrows fields have comments about the possible values, but there is no checking. They should be properties instead, so you can enforce these values.

Personally, I dislike the ToString method. It should be used solely for debugging purposes. That is because it has no semantic value, so your formatting methods should have another name. That said, your implementation simply looks like it was made so your debugging/immediate window would be easier to read, so I recommend removing it later, when you don't need it anymore.

null as X is still null, so your Equals method can be reduced to:

public override bool Equals(object other) {
    Room otherRoom = other as Room;
    return otherRoom != null && roomId == otherRoom.roomId;
}

You have a strange comment on the Equals saying "Shoullddddd be good enough...". I don't know the answer, but it is a strange design anyway: what does a room uses its id for? Is it really a data pertaining to this object? Or it is used only for places outside its scope?

Furthermore, you GetHashCode uses only roomId to generate the hash, but this value is mutable. Never ever use mutable values on GetHashCode. You can find some discussion about the topic in this SO question and some general discussion about GetHashCode in Eric Lippert's blog.

The variable naming is also a bit weird, with the need of comments to clarify things. That is never a good thing. Also, your Cave.AddRoom method has additional rules for the room: you can't have bats and pit, and each room must be accessible. And about this rule of bats and pit, you could design it as an enum. And your adjacent rooms could have a more generic type. All that said, I would've probably written your room as:

```
public class Room {

public enum RoomDangers {
None,
Bats,
Pit
}

private int gold;
private int arrows;
private IEnumerable adjacentRooms;

public RoomDangers Dangers { get; set; }

public int Gold {
get {
return gold;
}
set {
if (value = 0");
gold = value;
}
}

public int Arrows {
get {
return arrows;
}
set {
if (value = 0");
arrows = value;
}
}

public IEnumerable AdjacentRooms {
get {
return adjacentRooms;
}
set {
if (value == null)
throw new ArgumentNullException("value");
if (!value.Any())
t

StackExchange Code Review Q#87898, answer score: 6