Updating string array in function - C

Question

I am writing C function, which updates array of strings, only if given elements are unique. I have implemented this function like this:

char *cities[100];

/* adds 2 names if they're unique instances and updates total number of cities names
if necessary */

int addCity(char* city1, char* city2, char** arr, int amount) {
    int toReturn = amount;
    int i;
    int flag1 = 0;
    int flag2 = 0;

    // checking whether first city already exists
    for (i = 0; i < amount; i++ ) {
        if (strcmp(arr[i], city1) == NULL) {
            flag1 = 1;
            break;
        }
    }
    if (flag1 == 0) {
        arr[amount] = city1;
        toReturn++;
    }
    // 2nd city
    for (i = 0; i < amount; i++ ) {
        if (strcmp(arr[i], city2) == NULL) {
            flag2 = 1;
            break;
        }
    }
    if (flag2 == 0 && flag1 == 1) {
        arr[amount] = city2;
        toReturn++;
    }
    if (flag2 == 0 && flag1 == 0) {
        arr[amount+1] = city2;
        toReturn++;
    }
    return toReturn;
}

It looks that I get some warnings(comparison between pointer and integer) when trying to compare element of String array and String itself. How can I get rid of that? And overall, what else can be improved in this function?

In addition, I doubt about arr[amount] = city1, but when i use strcpy() program does not work at all.

There is no string type in C. It is an array of char with specific conventions. However, your code does not use an array of that, but pointers. A pointer is not an array (nor a "string"). — too honest for this site
– too honest for this site, Commented May 18, 2016 at 19:39
@Unknown It is unclear if the two cities are equal each other do you need to add one of them? — Vlad from Moscow
– Vlad from Moscow, Commented May 18, 2016 at 19:52
@Olaf I understand that. I could say I use array of pointers to char, I just named it so to be simplier to read :) — Unknown
– Unknown, Commented May 18, 2016 at 19:57
@VladfromMoscow Basically, I give to the function two cities which are NOT equal and I have to include them both (if they're not existing in array yet). — Unknown
– Unknown, Commented May 18, 2016 at 20:00

Vlad from Moscow · Accepted Answer · 2016-05-18 20:28:13Z

It seems what you need is the following

char *cities[100];

/* adds 2 names if they're unique instances and updates total number of cities names
if necessary */

int addCity( const char *city1, const char *city2, char **arr, int amount ) 
{
    int i;

    // checking whether first city already exists
    i = 0;
    while ( i < amount && strcmp( arr[i], city1 ) != 0 ) i++;

    if ( i == amount )
    {
        arr[amount] = malloc( strlen( city1 ) + 1 );
        if ( arr[amount] ) strcpy( arr[amount++], city1 );
    }

    // 2nd city
    i = 0;
    while ( i < amount && strcmp( arr[i], city2 ) != 0 ) i++;

    if ( i == amount )
    {
        arr[amount] = malloc( strlen( city2 ) + 1 );
        if ( arr[amount] ) strcpy( arr[amount++], city2 );
    }

    return amount;
}

As for your code then these conditions in the if statements

if (strcmp(arr[i], city1) == NULL)
if (strcmp(arr[i], city2) == NULL)

are wrong. There should be

if (strcmp(arr[i], city1) == 0)
if (strcmp(arr[i], city2) == 0)

Also after this code block

if (flag1 == 0) {
    arr[amount] = city1;
    toReturn++;
}

you should also increase amount because it is used below as an index in the array of pointers.

These two conditions

if (flag2 == 0 && flag1 == 1) {
if (flag2 == 0 && flag1 == 0) {

are equivalent to condition

if (flag2 == 0) {

And it seems you should dynamically allocate memory for each added city.

As for me then I would use the following order of the function parameters

int addCity( char **arr, int amount, const char *city1, const char *city2 );

Or even the function could be defined the following way

int addCity( char **arr, int amount, const char *city )
{
    int i;

    // checking whether the city already exists
    i = 0;
    while ( i < amount && strcmp( arr[i], city ) != 0 ) i++;

    if ( i == amount )
    {
        arr[amount] = malloc( strlen( city ) + 1 );
        if ( arr[amount] ) strcpy( arr[amount++], city );
    }

    return amount;
}

and called separatly two times for each added city.

4386427 · Accepted Answer · 2016-05-18 20:05:04Z

1

First strcmp return an integer. So don't compare it to NULL. NULL is a pointer value. So instead do a compare to 0.

if (strcmp(arr[i], city1) == 0)

Your main problem is however that you assign pointers passed as arguments into your cities. That is a major problem as the pointers will (likely) go out of scope soon.

You need to allocate memory and then do a strcpy. Something like:

if (flag1 == 0) {
    char* tmp = malloc(strlen(city) + 1); // add 1 for zero termination
    strcpy(tmp, city);
    arr[amount] = tmp;
    toReturn++;
}

And... don't forget to free the memory when done with it.

answered May 18, 2016 at 20:05

4386427

44.7k4 gold badges44 silver badges69 bronze badges

1 Comment

Jonathan Leffler Over a year ago

And don't forget to check for successful allocation before copying. Or consider using strdup().

R Sahu · Accepted Answer · 2016-05-18 19:30:55Z

0

    if (strcmp(arr[i], city1) == NULL) {

should be

    if (strcmp(arr[i], city1) == 0 ) {

Similarly, use

    if (strcmp(arr[i], city2) == 0 ) {

Whether you should use

    arr[amount] = city1;

or

    strcpy(arr[amount], city1);

depends on how memory for those two objects are managed. It's hard to suggest anything on that without seeing the rest of your code.

answered May 18, 2016 at 19:30

R Sahu

207k15 gold badges168 silver badges291 bronze badges

3 Comments

Unknown Over a year ago

I am reading these (city1, city1) from file while !EOF and calling this function everytime. My intention is that each name would be represented in array without repetition. Maybe I should use memmory allocation and strcpy in this case?

R Sahu Over a year ago

@Unknown, that is useful information but not enough to suggest a solution.

Jonathan Leffler Over a year ago

Another distinctly plausible alternative treatment is arr[amount++] = strdup(city1); — but there isn't enough information to be sure that it is correct either.

Collectives™ on Stack Overflow

Updating string array in function - C

3 Answers 3

Comments

1 Comment

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Comments

1 Comment

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related