1

This code stores the number of words in an integer variable and the words in a multidimensionnel string then sorts the words alphabetically using a function. The problem I have is in the function call.

#include<stdio.h>
#include<string.h>
void sort(char*[50],int);
int main ()
{
    int i,n=0;
    char s[50][10];

    scanf("%d",&n);//scaning the number of words

    for(i=0;i<=n;i++)//scaning the words
    gets(s[i]);

    sort(s,n);

    for(i=0;i<=n;i++)//printing the words sorted
    printf("%s\n",s[i]);
}
void sort(char*s[50],int n)
{
    int i,j,cmp;
    char tmp[1][10];

    //bubble sorting of words
    for(i=0; i<n; i++)
        for(j=0; j<n-1; j++)
        {
            cmp=strcmp(s[j],s[j+1]);

            if(cmp>0)
            {
                strcpy(tmp[0],s[j+1]);
                strcpy(s[j+1],s[j]);
                strcpy(s[j],tmp[0]);
            }   
        }
}
6
  • 1
    This revised code can't compile cleanly; the declaration of sort() takes different types from the definition of sort. Using a temporary array of size 10 to copy strings of size 50 seems likely to lead to trouble too. Commented May 19, 2013 at 17:04
  • What Jonathan said. Also, initialize n, so you won't overflow the buffer. Commented May 19, 2013 at 17:08
  • 4
    Also, do not use gets()! Never use gets(). Pretend it aborts your program. It should be implemented as doing that, in fact, because using it will lead to problems. The first internet worm, the Morris worm from 1988, used gets() to break in! Commented May 19, 2013 at 17:12
  • @JonathanLeffler, I still have an error in the function call. Can you please correct it. Thank you for the answer! Commented May 19, 2013 at 17:56
  • 2
    Parentheses are important. void sort(char*[50],int); is quite different from void sort(char (*)[50], int); [Note that after your change of array dimensions, it should be char (*)[10].] And in the definition, void sort(char (*s)[10], int n) { /* body */ }. Also note that for(i=0;i<=n;i++) should be for(i = 0; i < n; i++). Commented May 19, 2013 at 18:01

3 Answers 3

3

Turn on your warnings. char s[50][50] is not convertible to char*.

Try this:

void sort(char(*)[50],int);

This tells the compiler you pass in a pointer to at least one buffer of 50 characters. This is what multidimensional arrays decay to when passed into functions.

And to further rid you of the silly notion that "char[] is the same as char*", which for some reason is still taught everywhere, read this: http://c-faq.com/aryptr/aryptr2.html

Sign up to request clarification or add additional context in comments.

8 Comments

I changed the code, but it is still not working. Thank you for answering my question.
@JohnnyCash, changed it how? Please update your question with the changes.
I changed the declaration of the function.
@JohnnyCash, I could. But my boss wants me to do something similar, and he's paying. I will advise you to learn how to use a debugger. Step through the code and fix it. It'll make you a far better developer than asking others to correct your code for you.
thank you for helping me to correct my code. Now, I still have one problem with the function call. Can you please tell me what is incorrect in the function call?
|
1

You're close. Here's a working version of your code. Note that the scanf() leaves the newline from the number for the ... fgets(), because you won't ever use gets() again, will you? ... so you need to read up to and including the newline after the scanf(). Of course, there's the interesting question of 'why do people think it is better for humans to count than to make computers count instead?' It would be more sensible not to bother with the count. Note that the revised code validates n to ensure it is not larger than 50.

Revised code that works on lines of length 9 or shorter

#include <assert.h>
#include <stdio.h>
#include <string.h>

void sort(char s[50][10], int);

int main(void)
{
    int i;
    int n = 0;
    char s[50][10];
    char line[11];

    if (scanf("%d", &n) != 1)
    {
        fprintf(stderr, "Failed to read a number\n");
        return 1;
    }
    if (n <= 0 || n > 50)
    {
        fprintf(stderr, "%d is out of the range 1..50\n", n);
        return 1;
    }
    // Gobble rest of first line
    while ((i = getchar()) != EOF && i != '\n')
        ;

    for (i = 0; i < n; i++)
    {
        if (fgets(line, sizeof(line), stdin) == 0)
            break;
        // Remove newline from input
        size_t len = strlen(line);
        assert(len > 0 && len <= sizeof(s[i]));
        line[len-1] = '\0';
        strcpy(s[i], line);
    }
    n = i;  // In case the file was shorter than stated!

    printf("Before:\n");
    for (i = 0; i < n; i++)
        printf("%s\n", s[i]);

    sort(s, n);

    printf("After:\n");
    for (i = 0; i < n; i++)
        printf("%s\n", s[i]);

    return 0;
}

void sort(char s[50][10], int n)
{
    int i, j, cmp;
    char tmp[10];

    if (n <= 1)
        return; // Already sorted

    for (i = 0; i < n; i++)
    {
        for (j = 0; j < n-1; j++)
        {
            cmp = strcmp(s[j], s[j+1]);

            if (cmp > 0)
            {
                strcpy(tmp, s[j+1]);
                strcpy(s[j+1], s[j]);
                strcpy(s[j], tmp);
            }
        }
    }
}

This code reads lines into a string long enough to take up to 9 data characters, a newline, and the terminal null. It removes the newline, leaving up to 9 data characters and a terminal null.

Sample run:

Before:
Number 34
Number 39
Number 32
Number 30
Number 22
Number 34
Number 57
Number 28
Number 30
Number 47
Number 43
Number 23
Number 22
After:
Number 22
Number 22
Number 23
Number 28
Number 30
Number 30
Number 32
Number 34
Number 34
Number 39
Number 43
Number 47
Number 57

Original code that works on lines of length 8 or shorter

#include <assert.h>
#include <stdio.h>
#include <string.h>

void sort(char s[50][10], int);

int main(void)
{
    int i;
    int n = 0;
    char s[50][10];

    if (scanf("%d", &n) != 1)
    {
        fprintf(stderr, "Failed to read a number\n");
        return 1;
    }
    if (n <= 0 || n > 50)
    {
        fprintf(stderr, "%d is out of the range 1..50\n", n);
        return 1;
    }
    // Gobble rest of first line
    while ((i = getchar()) != EOF && i != '\n')
        ;

    for (i = 0; i < n; i++)
    {
        if (fgets(s[i], sizeof(s[i]), stdin) == 0)
            break;
        // Remove newline from input
        size_t len = strlen(s[i]);
        assert(len > 0);
        s[i][len-1] = '\0';
    }
    n = i;  // In case the file was shorter than stated!

    printf("Before:\n");
    for (i = 0; i < n; i++)
        printf("%s\n", s[i]);

    sort(s, n);

    printf("After:\n");
    for (i = 0; i < n; i++)
        printf("%s\n", s[i]);

    return 0;
}

void sort(char s[50][10], int n)
{
    int i, j, cmp;
    char tmp[10];

    if (n <= 1)
        return; // Already sorted

    for (i = 0; i < n; i++)
    {
        for (j = 0; j < n-1; j++)
        {
            cmp = strcmp(s[j], s[j+1]);

            if (cmp > 0)
            {
                strcpy(tmp, s[j+1]);
                strcpy(s[j+1], s[j]);
                strcpy(s[j], tmp);
            }
        }
    }
}

The 'big' change is the way the function array parameter is declared and defined. You're passing an array of 50 rows with 10 characters per row, so simply specify that in the function. You could drop the 50 from the dimensions of the function parameter with no change to the program's behaviour.

Sample input:

8
fed
abc
cba
def
hij
cba
xyz
aaa

Example run:

$ ./srt < data
Before:
fed
abc
cba
def
hij
cba
xyz
aaa
After:
aaa
abc
cba
cba
def
fed
hij
xyz
$

The fact that this needed revising shows the importance of testing the limits (and carefully defining the limits).

The code as revised is still not general purpose code. The fixed limit of at most 50 lines of input, the count of the number of lines required as part of the input, and the fixed line length of at most 10 characters per line all make it toy code. As such, GIGO (garbage in, garbage out) is not an unreasonable reaction. If the data file contains overlong lines, you get what you get. The code won't crash, but the output may not make much sense.

Comments

0

change

void sort(char*[50],int);
...
void sort(char*s[50],int n)

to

void sort(char(*)[10],int);//not char(*)[50]
...
void sort(char(*s)[10],int n)

and

//remain newline
scanf("%d",&n);

to

scanf("%d%*c",&n);//read and drop newline

so change

for(i=0;i<=n;i++)

to

for(i=0;i<n;i++)

or

char *p[50];
for(i=0;i<n;++i)
    p[i]=&s[i][0];
sort(p,n);//OK only exchange of pointer in this case

Comments

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.