tags:

views:

270

answers:

6

I am not a C programmer. I have just started reading K&R's TCPL last week. I have written this 42 line code in Java. I tried converting it to C, but it is giving me a segmentation fault.

Here is the Java version: http://codepaste.net/m8jz6m

My failed attempt to port it to C:

//Not working.
#include <stdlib.h>
#include <string.h>
 
void caesar ( const int SIDE )
{    
    int array [SIDE] [SIDE] ;

    for ( int number = 1; number <= SIDE * SIDE; ++ number )
        array [ getY ( number, SIDE ) ] [ getX ( number, SIDE ) ] = number ;

    for ( int Y = 0; Y < SIDE; ++ Y ) {
        for ( int X = 0; X < SIDE; ++ X)
            printf ( sprintf ("%%%dd" , strlen(itoa(SIDE*SIDE))), array [Y] [X] );
        printf ("\n");
    }
}
 
int getX ( const int number, const int SIDE )
{
    return SIDE == 1 ? 0 : number > 4 * SIDE - 4 ? 1 + getX ( number - 4 * SIDE + 4, SIDE - 2 ) : number >= 3 * SIDE - 2 ? 0 : number >= 2 * SIDE - 1 ? 3 * SIDE - 2 - number : number > SIDE ? SIDE - 1 : number - 1 ;
}
 
int getY ( const int number, const int SIDE )
{
    return SIDE == 1 ? 0 : number > 4 * SIDE - 4 ? 1 + getY ( number - 4 * SIDE + 4, SIDE - 2 ) : number >= 3 * SIDE - 2 ? 4 * SIDE - 3 - number : number  >= 2 * SIDE - 1 ? SIDE - 1 : number > SIDE ? number - SIDE : 0 ;
}
 
void main ( int argc, char *argv )
{
    if ( argc == 0 )
        printf ("\tUsage: java Caesar [side] | side:> Length and breadth of the square.\n");
    else
        caesar ( atoi( argv[1] ) );
}
 
 void itoa(int n, char s[])
 {
     int i, sign;

     if ((sign = n) < 0)  /* record sign */
         n = -n;          /* make n positive */
     i = 0;
     do {       /* generate digits in reverse order */
         s[i++] = n % 10 + '0';   /* get next digit */
     } while ((n /= 10) > 0);     /* delete it */
     if (sign < 0)
         s[i++] = '-';
     s[i] = '\0';
     reverse(s);
 }

  /* reverse:  reverse string s in place */
 void reverse(char s[])
 {
     int i, j;
     char c;

     for (i = 0, j = strlen(s)-1; i<j; i++, j--) {
         c = s[i];
         s[i] = s[j];
         s[j] = c;
     }
 }

Please let me know where I am going wrong.

+6  A: 
  • You are calling your itoa() function with one argument, while the function actually expects two.

  • Also, it seems that this line:

    array [ getY ( number, SIDE ) ] [ getX ( number, SIDE ) ] = number ;
    

    could be better expressed with something like:

    array[number / SIDE][number % SIDE] = number;
    

    without using those getX and getY functions (that are curiously recursive). I haven't examined those functions in detail to see what they actually do, but you may consider the above modification in any case.

  • sprintf() is called with incorrect arguments. The first parameter should be the destination buffer. You seem to be using it as if it returned a string (it doesn't).

  • Look up the function of * inside a printf format specifier.

Greg Hewgill
What the code is supposed to do:> java Caesar 3 OUTPUT:1 2 38 9 47 6 5(EDIT: That' wasn't worth it. Didn't know \n's get removed. There is one \n after each 3 numbers in the output.--An inward - spiralling array.
Angad
* each = every.
Angad
I see. Well ignore my second point then.
Greg Hewgill
+1  A: 

Memory management is often the Achilles' Heel. Read the parts about malloc() and free(). Look for problem spots where you handle strings and arrays.

Yuval F
A: 

Yes I second Greg and Yuval's answers. In particular you need to allocate some space before you call itoa(). Where is itoa storing its output? You are then calling strlen on the output of itoa. but itoa returns void!

int n,x;
...
char* outpt = (char*)malloc(30);
itoa(x, outpt);
n=strlen(outpt);

Also, you should really pass a third parameter to itoa: the buffer length. If the method generates a very long nstring, you could be in trouble! kin itoa you should check after incrementing i each time whether it is greater than the buffer lengt

Sanjay Manohar
A: 

the critical line:

 printf ( sprintf ("%%%dd" , strlen(itoa(SIDE*SIDE))), array [Y] [X] );

the sprintf's first argumet shoulb be a string.

not the problem, but: i got very confused reading your code: in "C", uppercase names are for Constants (#defines), not for const typed variables.

itoa() is defeined with two parameters and invoked with one.

you are using much too many spaces.

it is "i++" and not "i ++" in C (not that it matters syntactically, but reading-wise).

Peter Miehle
Are there any official coding conventions for C? I always thought there weren't... Still, I also prefer using capital letters only for compile time constants...
Alderath
A: 

A few notes:

  • You're calling your own itoa function with 1 argument but you've declared it to take 2 arguments
  • You're taking the strlen() of your itoa function , but have declared it with a return type of void
  • Your use of sprintf is wrong, sprinf needs a buffer to place the result in.

Compile your program with warnings enabled, you should get warnings about most of these.

nos
A: 

the itoa you're calling is not the itoa you've declared. Comment out #include <stdlib.h> and you should get a compile error instead of just warnings.

Dave