7
\$\begingroup\$

I came across an interesting method of solution for the Tower of Hanoi puzzle and wrote a short version of it as a programming exercise.

The program produces the correct results but I have two questions.

First is there simpler way to write the alternating step of determining the only valid move which does not involve the smallest disk.

Second when I try to make the two primary routines (move smallest disk and make alternating move) into functions the handling of variables becomes unwieldy.

/* tower.c

   Tower of Hanoi -- mechanical solution

   Arrange the three rods to form a triangle.

   Alternate between moving the smallest disk and making the only valid move
   which does not involve the smallest disk.

   The following rule will make sure that the tower of disks end up on the
   destination rod: if the number of disks in the puzzle is an odd number
   always move the smallest disk counter-clockwise around the triangle; if
   the number of disks in the puzzle is an even number always move the
   smallest disk clockwise around the triangle.
*/

#include <stdio.h>
#include <stdbool.h>

int destinationCount (int array[], int numberOfElements)
{
    int count = 0, i;

    for ( i = 1; i <= numberOfElements; ++i )
        if ( array[i] == 3 )
            ++count;

    return count;
}

int main (void)
{ 
    int numberOfDisks, i, smallestDir, moveCount = 0;  
    bool everyOtherMove = false;
    int rodFrom, rodTo, disk;
    int topDisk[4];
    int temp;

    printf ("\nTower of Hanoi puzzle\n");

    printf ("\nnumber of disks? ");
    scanf ("%i", &numberOfDisks);

    int rod[numberOfDisks + 1];

    // all disks start on rod 1

    for ( i = 1; i <= numberOfDisks; ++i )
        rod[i] = 1;

    // set direction to move smallest disk

    if ( (numberOfDisks & 1) == 0 )
        smallestDir = 1;
    else
        smallestDir = -1;    

    printf("\nsolution\n\n");

    do {

        ++moveCount;

        if ( ! everyOtherMove ) {

            // move smallest disk

            rodFrom = rod[1];

            rodTo = rodFrom + smallestDir;
            if ( rodTo > 3 )
                rodTo = 1;
            if ( rodTo < 1 )
                rodTo = 3;

            disk = 1;

        }
        else {

            // make only valid move not involving the smallest disk

            // find disk at the top of each rod

            for ( i = 1; i <= 3; ++i )
                topDisk[i] = numberOfDisks + 1;

            for ( i = numberOfDisks; i >= 1; --i )
                topDisk[rod[i]] = i;            

            // find which disk to move

            switch ( rod[1] )
            {
                case 1:
                    rodFrom = 2;
                    rodTo = 3;
                    break;
                case 2:
                    rodFrom = 1;
                    rodTo = 3;
                    break;
                case 3:
                    rodFrom = 1;
                    rodTo = 2;
                   break;
                default:
                    printf ("error");
                    break;
            }            

            if ( topDisk[rodFrom] > topDisk[rodTo] ) {
                // swap values
                temp = rodFrom;
                rodFrom = rodTo;
                rodTo = temp;            
            }

            disk = topDisk[rodFrom]; 

        }    

        printf ("%i: disk %i rod %c to rod %c\n", moveCount, disk, 
                 rodFrom + 64, rodTo + 64);

        // move disk

        rod[disk] = rodTo;

        // toggle everyOtherMove

        everyOtherMove = ! everyOtherMove;

    }
    while ( destinationCount (rod, numberOfDisks) != numberOfDisks );

    return 0;
}   
\$\endgroup\$
3
\$\begingroup\$

Remove the everyOtherMove flag variable

You use it only once to control the logic of your program:

if ( ! everyOtherMove ) {

and this usage may be substitued with a evenness check of the moveCount variable that you would have anyway.

Do not declare vars so much before using them

At line 36 temp is declared as an int, at line 113 temp is used for the first time. How can the reader remember the type of temp 77 lines later? Declare it just before using it.

Loop variables should be declared inside the loop statement as C99 allows it.

Use ternary when it clearly simplifies

if ( (numberOfDisks & 1) == 0 )
    smallestDir = 1;
else
    smallestDir = -1;    

Becomes

int smallestDir = (numberOfDisks & 1) == 0 ? 1 : -1

Much shorter and without the smallestDir = repetition.

Using % instead of bitwise operations to check evenness would be a further improvement.

Reduce main (both code and vertical whitespace)

I introduce this helper function:

int wrap_around(int min, int max, int value)
{
    return value < min ? max : (value > max ? min : value);
}

(Please note that it could also be written with if conditionals, I wrote it like this just because of my familiarity with ternary).

The first if branch now is:

    if (moveCount % 2 == 0) {
        // move smallest disk
        rodFrom = rod[1];
        rodTo = wrap_around(1, 3, rodFrom + smallestDir);
        disk = 1;
    }

while before it was:

    if ( ! everyOtherMove ) {

        // move smallest disk

        rodFrom = rod[1];

        rodTo = rodFrom + smallestDir;
        if ( rodTo > 3 )
            rodTo = 1;
        if ( rodTo < 1 )
            rodTo = 3;

        disk = 1;

    }

The same concept is expressed in much less space, and this is a good attribute in my view because:

  • Some logic is modularized in other functions, the reader gets a more abstract overview.
  • If all code is compacted this way an overall view of the program becomes possible helping understanding.

Compacting the else clause

    else {

        // make only valid move not involving the smallest disk

        // find disk at the top of each rod

        for ( i = 1; i <= 3; ++i )
            topDisk[i] = numberOfDisks + 1;

        for ( i = numberOfDisks; i >= 1; --i )
            topDisk[rod[i]] = i;            

        // find which disk to move

        switch ( rod[1] )
        {
            case 1:
                rodFrom = 2;
                rodTo = 3;
                break;
            case 2:
                rodFrom = 1;
                rodTo = 3;
                break;
            case 3:
                rodFrom = 1;
                rodTo = 2;
               break;
            default:
                printf ("error");
                break;
        }            

        if ( topDisk[rodFrom] > topDisk[rodTo] ) {
            // swap values
            temp = rodFrom;
            rodFrom = rodTo;
            rodTo = temp;            
        }

        disk = topDisk[rodFrom]; 

    }    

I do not fully understand the uppermost two for loops but both the switch and the body of the if ( topDisk[rodFrom] > topDisk[rodTo] ) statement are performing very clear, specific tasks, so:

    else {
        // make only valid move not involving the smallest disk
        // find disk at the top of each rod
        for ( i = 1; i <= 3; ++i )
            topDisk[i] = numberOfDisks + 1;
        for ( i = numberOfDisks; i >= 1; --i )
            topDisk[rod[i]] = i;            

        // find which disk to move
        find_start_and_destination(rod[1], *rodFrom, *rodTo);
        if ( topDisk[rodFrom] > topDisk[rodTo] ) {
            SWAP(rodFrom, rodTo);    
        }
        disk = topDisk[rodFrom]; 

    }    

I just removed the unnecessary blanklines (blanklines should separate logically separated blocks of code, not each line / statement), and used a function to incorporate the switch and a macro to swap variables. The function must use pointers because two values may not be returned from a function in C, but I think the modularization is still an advantage.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the review. I use the toggled everyOtherMove flag to emphasize the alternating nature of the algorithm. The bitwise AND operator -- instead of the MOD operator -- reflects a bit of background in assembly language. Also, I've always found the conditional (ternary) operator cryptic -- although it does have a nice underlying if-then-else structure. The uppermost 2 for loops above the switch statement just calculate the smallest disk on each of the 3 rods. I genuinely appreciate the comments. \$\endgroup\$ – ringzero Aug 6 '16 at 22:18
  • \$\begingroup\$ @ringzero The flag is another variable to initialise maintain and keep track of, it adds complexity while not being necessary. % is just more intuitive (remainder from division) and the compiler can optimiza it anyway. And ternary is very underrated in my opinion conditon ? Iftrue : Iffalse that's all, nesting them may get complex but a single one is clear. Ok thanks for explaining those loops. Thanks to you too for the fine question. \$\endgroup\$ – Caridorc Aug 6 '16 at 22:20
1
\$\begingroup\$

This self-answer contains an improved solution and incorporates several items suggested by Caridorc.

The changes to the original version include reduced vertical whitespace, declaring variables just before use as well as declaring loop variables inside loops as allowed in C99 programs.

Also the 2 primary routines, move smallest disk and move alternate disk, are rewritten as functions. The functions return which disk to move. Pointers are used to keep track of which rods are involved in moving the disk.

These changes make the resulting program simpler and clearer.

edit: used % (MOD) instead of both the Boolean variable and the bit-wise AND

/* tower.c

   Tower of Hanoi -- mechanical solution

   Place one of the three rods upright at each corner of a triangle.

   Alternate between moving the smallest disk and making the only valid move
   which does not involve the smallest disk.

   The smallest disk always moves in the same direction: counter-clockwise
   if there are an odd number of disks in the puzzle; clockwise if there are
   an even number of disks in the puzzle.
*/

#include <stdio.h>

int moveSmallestDisk (int rod[], int smallestDir, int *ptrFrom, int *ptrTo)
{
    *ptrFrom = rod[1];
    *ptrTo = *ptrFrom + smallestDir;
    if ( *ptrTo > 3 )
        *ptrTo = 1;
    if ( *ptrTo < 1 )
        *ptrTo = 3;
    return 1;
}

int moveAlternateDisk (int rod[], int numberOfDisks, int *ptrFrom, int *ptrTo)
{
    // determine which disk is at the top of each rod
    int topDisk[4];
    for ( int i = 1; i <= 3; ++i )
        topDisk[i] = numberOfDisks + 1;
    for ( int i = numberOfDisks; i >= 1; --i )
        topDisk[rod[i]] = i;

    switch (rod[1])
    {
        case 1:
            *ptrFrom = 2;
            *ptrTo = 3;
            break;
        case 2:
            *ptrFrom = 1;
            *ptrTo = 3;
            break;
        case 3:
            *ptrFrom = 1;
            *ptrTo = 2;
            break;
        default:
            printf ("error");
            break;
    }
    if ( topDisk[*ptrFrom] > topDisk[*ptrTo] ) {
        int temp;
        temp = *ptrFrom;
        *ptrFrom = *ptrTo;
        *ptrTo = temp;
    }

    return topDisk[*ptrFrom];
}

int destinationCount (int rod[], int numberOfDisks)
{
    int count = 0;

    for ( int i = 1; i <= numberOfDisks; ++i ) {
        if ( rod[i] == 3 )
            ++count;
    }

    return count;
}

int main (void)
{
    printf ("\nTower of Hanoi puzzle\n\n");
    printf ("number of disks? ");
    int numberOfDisks;
    scanf ("%i", &numberOfDisks);

    int smallestDir;
    if ( (numberOfDisks % 2) == 0 )
        smallestDir = 1;
    else
        smallestDir = -1;
    int rod[numberOfDisks + 1];
    // all disks start on the first rod
    for ( int i = 1; i <= numberOfDisks; ++i )
        rod[i] = 1;

    printf ("\nsolution\n\n");
    int moveCount = 0, disk, rodFrom, rodTo;

    do {
        ++moveCount;

        if ( (moveCount % 2) == 1 )
            disk = moveSmallestDisk (rod, smallestDir, &rodFrom, &rodTo);
        else
            disk = moveAlternateDisk (rod, numberOfDisks, &rodFrom, &rodTo);

        printf ("%i: disk %i rod %c to rod %c\n", moveCount, disk,
               rodFrom + 64, rodTo + 64);

        rod[disk] = rodTo;
    }
    while ( destinationCount (rod, numberOfDisks) != numberOfDisks );

    return 0;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.