7
\$\begingroup\$

I'm trying to write the Josephus algorithm as a project for school, however, I think that there is a better way than what I already have.

public static int[] StartTheGame(int prisoners, int count)
{
    var position = count - 1;
    int[] list = new int[prisoners]; 
    for (int i = 0; i < prisoners; i++)
    {
        list[i] = 1;
        Console.ForegroundColor = ConsoleColor.Green;
        Console.Write(" {0} ", list[i]);
    }
    Console.WriteLine();
    for (int i = 1; i <= prisoners - 1; i++)
    {
        while (position + 1 >= prisoners)
        {
            position = (position) - prisoners;
        }
        if (list[position + 1] == 0)
        {
            position++;
        }
        if (list[position + 1] == 1)
        {
            list[position + 1] = 0;
        }
        position = position + count + 1;

        ShowInConsole(prisoners, list);
    }
    return list;
}

Any ideas to make it better?

The output for 10 and 2 is:

josephus algorithm output for 10 and 2

Full code:

private static int _prisoners, _count;

    static void Main()
    {
        while (true)
        {
            Console.ForegroundColor = ConsoleColor.White;
            GetInitialValues();

            int[] lastManAlive = StartTheGame(_prisoners, _count);

            Console.ForegroundColor = ConsoleColor.Green;
            Console.WriteLine("The last person who is freed is standing at position {0}. ", IsFreed(lastManAlive));

        }
    }

    private static void GetInitialValues()
    {
        Console.WriteLine("Please enter the number of prisoners that are going to be hanged: ");
        try
        {
            _prisoners = (int) Convert.ToInt64(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine("Something went wrong, please try again");
            GetInitialValues();
        }
        Console.WriteLine("Please enter a number less than the number of prisoners and greater than 0 to start the game and hang the prisoners: ");
        try
        {
            _count = (int)Convert.ToInt64(Console.ReadLine());
        }
        catch (Exception)
        {
            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine("Something went wrong, please try again");
            GetInitialValues();
        }
        while (_count <= 0)
        {
            Console.WriteLine("Please enter a number greater than 0 to start the game and hang the prisoners: ");
            _count = (int)Convert.ToInt64(Console.ReadLine());
        }
    }

    public static int[] StartTheGame(int prisoners, int count)
    {
        var position = count - 1;
        int[] list = new int[prisoners];
        InitiateGame(list, prisoners);
        for (int i = 1; i <= prisoners - 1; i++)
        {
            while (position + 1 >= prisoners)
            {
                position = (position) - prisoners;
            }
            if (list[position + 1] == 0)
            {
                position++;
            }
            if (list[position + 1] == 1)
            {
                list[position + 1] = 0;
            }
            position = position + count + 1;

            ShowInConsole(prisoners, list);
        }
        return list;
    }

    private static void InitiateGame(int[] list, int prisoners)
    {
        for (int i = 0; i < prisoners; i++)
        {
            list[i] = 1;
            Console.ForegroundColor = ConsoleColor.Green;
            Console.Write(" {0} ", list[i]);
        }
        Console.WriteLine();
    }

    public static int IsFreed(int[] list)
    {
        var length = list.Length;
        for (int i = 1; i <= length; i++)
        {
            if (list[i - 1] == 1)
            {
                return i;
            }
        }
        return -1;
    }

    public static void ShowInConsole(int prisoners, int[] list)
    {
        Console.WriteLine();
        for (int j = 0; j < prisoners; j++)
        {
            if (list[j] == 0)
            {
                Console.ForegroundColor = ConsoleColor.Red;
            }
            else
            {
                Console.ForegroundColor = ConsoleColor.Green;
            }
            Console.Write(" {0} ", list[j]);
        }
        Console.WriteLine();
    }
\$\endgroup\$
2
  • \$\begingroup\$ Why Convert.ToInt64? \$\endgroup\$ Commented Nov 30, 2016 at 21:58
  • \$\begingroup\$ Separate out user input and output from the actual logic. \$\endgroup\$ Commented Dec 1, 2016 at 15:51

2 Answers 2

2
\$\begingroup\$

The question is, what is the goal of this task. If it is just return the position of the last man who will survive and your teacher doesn't care about the killing sequence, we can write recursive function like this (source):

static void Main(string[] args)
{
    int n = 10;
    int k = 2;
    int position = josephus(n, k);
    Console.WriteLine($"The survivor josephus({n},{k}) is {position}.");
}

static int josephus(int n, int k)
{
    if (n == 1)
        return 1;
    else
        return (josephus(n - 1, k) + k - 1) % n + 1;
}

Few comments regarding your solution:

Exception

Test your code with initial values 10, 9. Exception occurs - IndexOutOfRange.

I would avoid of method recursive calling in catch block in GetInitialValues(). It could cause unexpected behavior of your application.

Improvements

You can generate array with initial value of each element using Enumerable.

int[] prisoners = Enumerable.Repeat(1, prisonersCount).ToArray();

If you pass prisoners array to method, then you don't need to know prisoners count. You can simply work with prisoners.Length property.

You display the array in Console in two methods InitiateGame() and ShowInConsole(). Make just one method for this purpose.

Simplify expression from position = (position) - prisoners; to position -= prisoners;

Use foreach cycle if you don't need change array members. Simplify condition with ternary operator. You can use string interpolation when you format strings.

public static void ShowInConsole(int[] prisoners)
{
    Console.WriteLine();
    foreach (int prisoner in prisoners)
    {
        Console.ForegroundColor = (prisoner == 0) ? ConsoleColor.Red : ConsoleColor.Green;
        Console.Write($" {prisoner} ");
    }
    Console.WriteLine();
}

Method names

GetInitialValues() is void. Get says that method should return some initial values. The other case is StartTheGame() which returns int[] array. That is strange. IsFreed() returns int. Usually if I see method named like IsSomeAdjective(), I usually expect boolean return type. Consider use of Array.IndexOf(). Then the function can look like:

public static int GetFreedPosition(int[] prisoners)
{
   return Array.IndexOf(prisoners, 1) + 1;
}

Variable names

variable list is in reality array int[]. I would call it prisoners. And the prisoners in fact represents prisoners count, so why don't call it prisonersCount.

Parsing, Casting

Paparazzi already asked the question about converting of string to Int64. Your variables are System.Int32 structures, so you don't need to convert to Int64 and then explicitly cast to Int32. Simply use Convert.ToInt32(Console.ReadLine()). Alternatives are methods int.Parse() or int.TryParse().

\$\endgroup\$
1
\$\begingroup\$

As your code is currently written, hold a int token for prisoners still alive, and for each step do:

position[(index+=steps+1)%position.length] = 0;

repeat until 1 is standing or no one has died in prisoner_count steps.

(Aren't you supposed to skip over the dead bodies? In your example output, in the last 4 lines you are just executing the remaining prisoners in sequence. I can't run it right now, but if that is the output for 10 and 2, then 10 and 1 should get into an infinite loop)

\$\endgroup\$
7
  • \$\begingroup\$ how so an infinite loop? the game is to count the prisoners, and kill the next one, so for 10 and 2, the third one dies, the 6th guy dies, the 9th guy dies, and then the end of the array is reached, so the 2nd guy dies and so on till 1 is left alive and is freed. for 10 and 1, the second one dies and so on. \$\endgroup\$ Commented Nov 30, 2016 at 20:59
  • \$\begingroup\$ by the way, the list is array, and the position is just an int. so I think you meant: list[(index+=steps+1)%list.length] = 0; I will check this, thanks \$\endgroup\$ Commented Nov 30, 2016 at 21:01
  • \$\begingroup\$ @AliRAN 10-2 is hitting 3-6-9-2-5-8-1-4-7-(10) (no repeat because lease common denominator is 30) 10-1 than should hit 2-4-6-8-10-(repeating) (least common denominator is 10) \$\endgroup\$ Commented Nov 30, 2016 at 21:02
  • \$\begingroup\$ @AliRAN My understanding is that 10-2 should be 3-6-9-2-7-1-8-5-10-(4) (when you don't count the dead bodies in each step) \$\endgroup\$ Commented Nov 30, 2016 at 21:05
  • \$\begingroup\$ the thing is, that while our teacher was explaining he was also counting the dead prisoners, but what you are saying makes more sense. but I don't get how you can implement that, since the counting is not dependent on the dead or alive prisoners. \$\endgroup\$ Commented Nov 30, 2016 at 21:25

Your Answer

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.