AI Decision Tree

I created little decision tree, what do you think about it, what I can correct? I wanted to create a code for tree that looks in that way:

My implementation:

```
using System;
using System.Collections.Generic;
using System.Text;

public interface TreeNode
{
bool nodeState { get; }
bool Evaluate();
}

///
/// Returns true when one of childs returns true
///
public class Selector : TreeNode
{
private List childNodes;
public bool nodeState { get; private set; } = false;

public Selector(List childNodes) { this.childNodes = childNodes; }

public bool Evaluate()
{
foreach (TreeNode node in childNodes)
if (node.Evaluate())
{
nodeState = true;
return true;
}
nodeState = false;
return false;
}
}

///
/// Returns true when all childs return true
///
public class Sequence : TreeNode
{
private List childNodes;
public bool nodeState { get; private set; } = false;

public Sequence(List childNodes) { this.childNodes = childNodes; }

public bool Evaluate()
{
foreach (TreeNode node in childNodes)
if (!node.Evaluate())
{
nodeState = false;
return false;
}
nodeState = true;
return true;
}
}

///
/// Has only one child, negate it
///
public class Inverter : TreeNode
{
private TreeNode nodeToInvert;
public bool nodeState { get; private set; } = false;

public Inverter(TreeNode nodeToInvert) { this.nodeToInvert = nodeToInvert; }

public bool Evaluate()
{
nodeState = !nodeToInvert.Evaluate();
return !nodeToInvert.Evaluate();
}
}

///
/// Leaf of tree, returns delegate of bool function that is setted in it's constuctor
///
public class ActionNode : TreeNode
{
public delegate bool ActionNodeDelegate();
private ActionNodeDelegate action;
public bool nodeState { get; private set; } = false;

Code style

Naming

-
The interface naming convention has a capital I at the beginning of your name so your TreeNode should be called ITreeNode.

-
Public members should follow the Pascal Case typing e.g instead of bool nodeState { get; } the name should be bool NodeState { get; }

-
Private variables should follow the camel Case typing e.g instead of Sequence Attack the name should be Sequence attack.

-
Boolean members should have a prefix like is, can, or something that will look like a question when you put it into an if statement take the following example in consideration:

if (KillMonster)
{
    //...
}
if (CanKillMonster)
{
    //...
}

The second one looks and reads a lot more clearer to me.

Do not omit curly braces, especially around a foreach loop

Func vs custom delegate

The difference between these 2 isn't that big, so if your needs don't match one of the following cases you can just use the generic functor:

-
You can have a specific name for your custom delegate, if you find it necessary, but not for a Func.

-
You can have ref / out parameters in a custom delegate but no in a Func.

That's pretty much it, they can both hold methods with the same signature, in your case I don't find a need for a custom delegate so you can just swap this line:

private ActionNodeDelegate action;

To this:

private Func action;

You can now delete your old delegate and replace the types everywhere needed.

Applying LINQ to make the code shorter and more readable

You have only 2 foreach loops in total and we can transform them into LINQ syntax:

foreach (ITreeNode node in childNodes)
{
    if (node.Evaluate())
    {
        NodeState = true;
        return true;
    }
}

foreach (ITreeNode node in childNodes)
{
    if (!node.Evaluate())
    {
        NodeState = false;
        return false;
    }
}

Can become:

if (childNodes.Any(node => node.Evaluate()))
{
    NodeState = true;
    return true;
}

if (childNodes.Any(node => !node.Evaluate()))
{
    NodeState = false;
    return false;
}

Avoiding multiple unnecessary calls

Why would you call a method twice, when you already have the return value saved?

NodeState = !nodeToInvert.Evaluate();
return !nodeToInvert.Evaluate();

NodeState = action();
return action();

Can become:

NodeState = !nodeToInvert.Evaluate();
return NodeState;

NodeState = action();
return NodeState;

It doesn't seems like a big improvement but it's basically making your code twice faster in this specific method. Imagine if Evaluate and action take long time, would you like to wait 20 seconds instead of just 10? The methods that will be executed and will control your AI decisions will be called probably each frame in a normal game, you want to make those as fast as possible.

Adding more modifiers to your variables

Access modifiers

You're being a little bit inconsistent with where you put access modifiers. Most of your variables in AITree have the explicit private access modifier but some of them don't I don't see a reason for that, they should all have it. Namely:

bool PlayerIsInAttackRange() => playerDistanceFromEnemy  playerDistanceFromEnemy  playerPower > 3;

Modifiers

A common modifier is readonly. This modifier allows you to give value to your variable only at initialization or in the constructor, this is pretty useful as it can guarantee that your variable wont mutate during it's lifetime.

Most of your variables can have readonly added:

private float playerDistanceFromEnemy;
private int playerPower;
private ActionNode IsInAttackRange;
private ActionNode IsVisible;
private Sequence Attack;
private Inverter Patrol;
private Sequence Escape;
private ITreeNode nodeToInvert;
private Func action;
private List childNodes;

Redundancy

You have some redundant variables in your AITree

These 2 for example:

private readonly ActionNode EstimatePlayerPower;
private readonly Selector Root;

You are using those only in your constructor as helper variables, but they have no other use in the class, you should either make them local variables to the constructor or just remove them.

Also in your ShowCommunicats method, the StringBuilder sb = new StringBuilder(); is absolutely redundant.

Overall design

-
A better way of saving the current state/action of your AI would be handy.

-
You should consider using the Strategy pattern as it's pretty good design pattern when it comes to making decisions.

Update

As requested I will put some guidance here on how to implement the Strategy pattern, but I cant provide a working example as your code currently isn't in that state yet, as it will require you to have actual actions to happen such as Attack, Patrol, etc.

You will need 2 things to implement the Strategy pattern:

-
Common base interface/abstract class which has the core signature / functionality.

-
Some enum which has all the possible action

StackExchange Code Review Q#153773, answer score: 9