patterncsharpMinor

Simple attempt to solve the travelling salesman prob

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

salesmansimpletheprobattempttravellingsolve

Problem

For practicing purposes, I created an application that attempts to find an optimal route between given points.

The algorithm is:

Connect two randomly selected points

Select a point that's still unconnected (let's call it A)

Find it's nearest connected neighbor (let's call it B, where C => B => D)

If the distance C => A is smaller than A => D save C => A => B => D, otherwise C => B => A => D




repeat while there're unconnected points



I don't want to discuss the algorithm, I know that it doesn't actually solve the TSP. But I'm interested in hints how to make my code more structured, easier to read and modify or anything else that could be improved.

Interfaces

/Interfaces/IMapCreator.cs

using TravellingSalesmanProblem.Classes;

namespace TravellingSalesmanProblem.Interfaces
{
    public interface IMapCreator
    {
        Map CreateMap(string data);
    }
}


/Interfaces/IMapOptimizer.cs

using TravellingSalesmanProblem.Classes;

namespace TravellingSalesmanProblem.Interfaces
{
    public interface IMapOptimizer
    {
        void Optimize(Map map);
    }
}


Main classes

/Classes/Map.cs

``
using System.Collections.Generic;
using System.Linq;
using TravellingSalesmanProblem.Interfaces;

namespace TravellingSalesmanProblem.Classes
{
public class Map
{
public IList Points { get; private set; }

public Map() : this(new List()) { }

public Map(IList points)
{
Points = points;
}

public Map AddPoint(Point point) {
Points.Add(point);
return this;
}

public Point GetPoint(int id)
{
return Points.Single(x => x.Id == id);
}

public void Optimize(IMapOptimizer optimizer)
{
optimizer.Optimize(this);
}

public float GetCostOfWholeRoute()
{
Point startingPoint = Points.First();
float cost = startingPoint.OutgoingConnection.Co

Solution

I am quite impressed, there's very little I'd change, well done.

I would however prefer to use var in local variable assignments when the right-hand side of the assignment makes the type obvious.

e.g.

Point a = new Point(0);

Should be

var a = new Point(0);

The reason I would prefer this, is that if I were to change the type of a I then only have to change it once.

Secondly, I don't like the single-letter variable names (with the exception of loop indices). They hide the intended purpose behind a cognitive leap. For your points, I'd recommend an array or collection of some kind, and for your connections, it may actually be more readable to omit them entirely and put the constructors into your AddPossibleConnection call.

public class StaticMapCreator : IMapCreator
    {

        private const int PointCount = 5;

        public Map CreateMap(string data)
        {
            var map = new Map();

            var points = new Point[PointCount];
            for(var i=0; i<PointCount; i++)
            {
                points.Add(new Point(i));
            }

            points[0].AddPossibleConnection(new Connection(points[0], points[1], 1))
                .AddPossibleConnection(new Connection(points[0], points[2], 1))
                .AddPossibleConnection(new Connection(points[0], points[3], 2))
                .AddPossibleConnection(new Connection(points[0], points[4], 5));

            //etc...

            foreach(var point in points)
            {
                map.AddPoint(point);
            }

            return map;
        }
    }

Lastly, there may be a simpler way of doing ids, instead of passing them in as parameters. Consider this code:

public class Point
    {
        private Guid id = Guid.NewGuid();
        public Guid Id
        {
            get
            {
                return id;
            }
        }

        //etc...

    }

Now whenever a new Point instance is created, it has its own unique identifier, without anything passed into a constructor, etc.

Lastly, if you're calling Map.GetPoint(int id) a lot, it might be better to store Points as a Dictionary where the key is the point's id. You can generate this easily by calling points.ToDictionary(x=>x.Id) or by adding the points as key-value pairs in AddPoint. The reason this is better is because a dictionary lookup is an O(1) operation, which means it will take the same time (barring hash collisions) to find a point in a dictionary with 10,000,000 items as it will a dictionary with 1 item. A List cannot promise that efficiency.

Code Snippets

Point a = new Point(0);

var a = new Point(0);

public class StaticMapCreator : IMapCreator
    {

        private const int PointCount = 5;

        public Map CreateMap(string data)
        {
            var map = new Map();

            var points = new Point[PointCount];
            for(var i=0; i<PointCount; i++)
            {
                points.Add(new Point(i));
            }

            points[0].AddPossibleConnection(new Connection(points[0], points[1], 1))
                .AddPossibleConnection(new Connection(points[0], points[2], 1))
                .AddPossibleConnection(new Connection(points[0], points[3], 2))
                .AddPossibleConnection(new Connection(points[0], points[4], 5));

            //etc...

            foreach(var point in points)
            {
                map.AddPoint(point);
            }

            return map;
        }
    }

public class Point
    {
        private Guid id = Guid.NewGuid();
        public Guid Id
        {
            get
            {
                return id;
            }
        }

        //etc...

    }

Context

StackExchange Code Review Q#67823, answer score: 5

Revisions (0)

No revisions yet.