0

So I have a char array that currently holds a struct of a person's information. For example,

struct Person{
char first_name[100];
char last_name[100];
char phone[13];
};

struct Person* arr[] = {("John", "Doe", "831.563.3642"), ("Julie", "Dee", "542.865.4644") };

And I am trying to have it end up printing to a file to look like this...

John, Doe, 831.563.3642
Julie, Dee, 542.865.4644

So far this is the code I have (to try and add the commas while putting into a file)

void print(FILE* file_out, struct Person* p1[]){
   size_t len = sizeof(p1) / sizeof(p1[0]);
   for(i = 0; i < len; i++){
      fprintf(file_out, "%s, %s, %s\n", p1[i].first, p1[i].last, p1[i].phone);
   }
}
return;
  • 2
    Not going to sugarcoat it: there's more wrong than right with this code. Do you have a [good book on the C language](https://stackoverflow.com/questions/562303/the-definitive-c-book-guide-and-list), because believe this: C is not a language you trial-and-error your way to success with. Regardless, if all you want is to print comma separators between data elements in your output, than why no do that. I.e., in the code that *prints* rather than manufacturing an otherwise-needless copy of a bunch of data? – WhozCraig Feb 16 '20 at 03:13
  • `void prnperson (struct Person *p) { printf ("%s, %s, %s\n", p->first_name, p->last_name, p->phone); }` – David C. Rankin Feb 16 '20 at 03:58
  • @WhozCraig I would have to have it print the first element (i.e. the name) then a comma, then the following elements in the same pattern would I not? I thought it would be easier to do it all in one function and return that. Also, I've been learning C for about a month so sorry about all the bugs. – CardBoardBox Feb 16 '20 at 04:16
  • Change to `void print(FILE* file_out, struct Person p1[]){ ..` as in the comment below my answer, no `'*'` in the declaration. Or `void print(FILE* file_out, struct Person *p1){ ..` with no `[..]` -- recall both `'*'` and `[..]` indicate 1-level of pointer indirection -- you only need 1-level for an array. – David C. Rankin Feb 16 '20 at 05:06

2 Answers2

0

struct Person* arr[1] is equivalent to Person arr is also equivalent to Person** arr I don't think that is what you meant. get rid of the pointer :D

Now some really important things about strings... The strlen function is the #1 reason for buffer overflow bugs - NEVER use strlen without some kind of limitation.

Also, internally string length looks like this...

int strlen(char* str) {
    int count = 0;
    while (str[count]) {
        count++;
    }
    return count;
}

notice that is almost your function? Your function could be changed to...

char* makeString(char* str) {
    char out[] = ""
    int count = 0;
    while (str[count]) {
        out[count] = str[count];
        count++;
    }
    out[count] = ',';
    out[count + 1] = 0;
    return out;
}

Lets talk about the major problems with this though - first, notice the while loop that only ends when it finds a 0? That is a GREAT way to make a bug. All somebody has to do is give a name just a little too long and boom! there is no ending 0.

Now imagine I call your function multiple times in a row... You are looping through the string each time!!! In other words, I iterate through the loop for the string "john". that is 4 times through the while loop. it also makes a char[6] to hold those things - since john is 4, and the ',' is 1, and the "end of the string" marker is a 0.

Now we add to the string again. we iterate over "john" the ",", and add "Joe". This means we need a new longer char[] of length 9. now we iterate over "john, " and "doe" for 9 while loop hits. Then we do this again and again. We iterate over john a HUGE number of times. Way too many times.

Now for the safe way to do this :D

//Create a string of a known length that can hold all the data
//Now we only need to create a char buffer once instead of multiple times
//In a regular computer, it is better to waste space than CPU time :D (in this case anyway...)
char printString[sizeof(arr.first_name) + 2 + sizeof(arr.last_name) + 2 + sizeof(arr.phone) + 1];
//Remember where we last wrote, and where we are in our current copy
int position = 0;
int i = 0;
//While it is not the end of the string, and we have not gone over the longest possible length...
while (arr[0].first_name[i] && i < sizeof(arr[0].first_name)) {
    //Copy the character
    printString[position] = arr.first_name[i];
    //Move to the next spot in both character buffers
    position++;
    i++;
}
//Add our comma and space
prinstString[position] = ',';
printtString[position + 1] = ' ';
position = position + 2;
int i = 0;
while (arr[0].last_name[i] && i < sizeof(arr[0].last_name)) {
    printString[position] = arr.last_name[i];
    position++;
    i++;
}
prinstString[position] = ',';
printtString[position + 1] = ' ';
position = position + 2;
int i = 0;
while (arr[0].phone[i] && i < sizeof(arr[0].phone)) {
    printString[position] = arr.phone[i];
    position++;
    i++;
}
prinstString[position] = 0;

Now for the benefits.

We only iterate through each string once. This avoids the problem above where we iterate over John a million times. (Look here to see a funny way to see it XD)

The biggest benefit is checking that we have not gone over the end. Nobody can exploit out strings anymore!!!

And also, we only need to alocate a char buffer ONCE to hold all this cumulative string concatenation stuff.

We also have some really nice information for the next guy if we wanted to pass it on, and notice it almost is the form of an algo? How awesome - it can almost be a function!

Now for the grand finale! All of that hard work has been done by someone so much smarter than both of us ;)

printf("%s, %s, %s", arr[0].first_name, arr[0].last_name, arr[0].phone);

I think I am done editing now!!

Watachiaieto
  • 417
  • 3
  • 10
  • Thank you for your edit attempt, but room for the `'\0'` is already provided in each of the original strings that is being combined. (you would technically only need `+2` for the `sprintf` buffer size since 2 of the 3 `'\0'` characters will be eliminated `:)` – David C. Rankin Feb 16 '20 at 04:59
  • @DavidC.Rankin I was considering a lack of a null terminator, but if the null terminator does not exist, It is extemely unlikely the next byte is a 0 anyway, so the sprintf will just keep on copying and overflowing the buffer anyway XD – Watachiaieto Feb 17 '20 at 03:48
  • If any of the strings `first_name`, `last_name` or `phone` isn't *nul-terminated* to begin with, then whether you use `+2, +4, or +5` wouldn't make much difference. The cause of the error would have occurred prior to the `sprintf` call and UB would be invoked with the `sprintf` call as a result. It's the old passing a non-terminated array to a string-function results in UB no matter how you slice it `:)` – David C. Rankin Feb 17 '20 at 04:04
0

You may be making things way harder on yourself than you need to. With your struct Person filled, in order to obtain the output you need, you simply need to output the struct members separated by a comma and space. A simple printf will do, e.g.

struct Person p1 = { "John", "Doe", "831.563.3642" };
...
printf ("%s, %s, %s", p1.first, p1.last, p1.phone);

The output from above would be:

John, Doe, 831.563.3642

You can never do char new[] = ""; -- which creates the array new of size 1 holding the '\0' character. (bad choice of names as well, new is a kewword in C++, its use as a variable name should be avoided). Once you declare and initialize the array with automatic storage duration of size 1 -- it's size is FIXED -- period, and cannot be expanded. You would need to dynamically allocate storage with malloc in order to be able to resize the storage later with realloc.

If you want to create an additional array holding the comma separated values, then you need to declare the array with storage to hold the largest first_name, last_name and phone initially, e.g. char buf[100 + 100 + 13 + 4]; (the +4 for the pair of ", " you want to insert as well) Now you can use sprintf to simply write to the new array.

Below is a short example which shows both the direct output with printf and the creation of a new array (buf) using sprintf. The results is the same regardless of whether you directly output the comma space separated values, or store them in a buffer first. The function csvperson() takes the destination array and a pointer to a filled struct as parameter to write the members of the struct separated by a comma and space to array, e.g.

#include <stdio.h>

#define MAXNM 100   /* if you need a constant, #define one (or mroe) */
#define MAXPH  13

typedef struct {            /* using a typedef allows you to later omit the */
    char first[MAXNM],      /* 'struct' prefix in your declarations and */
         last [MAXNM],      /* parameter lists */
        phone [MAXPH];
} person;

char *csvperson (char *dest, person *p)
{
    if (!dest)
        return NULL;

    sprintf (dest, "%s, %s, %s", p->first, p->last, p->phone);

    return dest;
}

int main (void) {

    char buf[2 * MAXNM + MAXPH + 4];
    person p1 = { "John", "Doe", "831.563.3642" };

    printf ("direct output : %s, %s, %s\nbuffer output : %s\n", 
            p1.first, p1.last, p1.phone, csvperson (buf, &p1));
}

Example Use/Output

$ ./bin/csvperson
direct output : John, Doe, 831.563.3642
buffer output : John, Doe, 831.563.3642

Look things over and let me know if you have further questions.


Array Example

To use the same methods above with an array, you would do:

int main (void) {

    char buf[2 * MAXNM + MAXPH + 4];
    person p1[] = { {"John", "Doe", "831.563.3642"},
                    {"Jane", "Doe", "832.563.3643"},
                    {"Mary", "Jane", "303.331.3333"} };
    int n = sizeof p1 / sizeof *p1;

    for (int i = 0; i < n; i++)
        printf ("\ndirect output : %s, %s, %s\nbuffer output : %s\n", 
                p1[i].first, p1[i].last, p1[i].phone, csvperson (buf, p1+i));
}

You can simply use fprintf to output to a file without buffering the output first, e.g.

fprintf (fileptr, "%s, %s, %s\n", p1[i].first, p1[i].last, p1[i].phone);

In general, don't worry about combining/concatenating/etc.. things you can simply write out (wherever be it a file, the terminal, etc..)

David C. Rankin
  • 81,885
  • 6
  • 58
  • 85
  • I understand what the printf is achieving, however, I am trying to write it into another file, not simply to the console so I'm trying to use fputs in order to achieve that. Would it still be something similar for that then? – CardBoardBox Feb 16 '20 at 04:30
  • Why not use `fprintf` to write to the file directly?? `fprintf (file_pointer, "%s, %s, %s", p1.first, p1.last, p1.phone);` Or, you can simply redirect the output from the program to a file, e..g `./a.out > newfile.txt` – David C. Rankin Feb 16 '20 at 04:30
  • The `'\n'` at the end just inserts a `newline` character. If you don't need a newline, omit it, if you want it, keep it in. C is a very exacting language, it only does what you tell it to - on a byte-by-byte basis -- so you have the *Ultimate* control `:)` (and the *Ultimate* responsibility to make sure it is done correctly) – David C. Rankin Feb 16 '20 at 04:34
  • It wants me to do something like ```p1[i]->first, p1[i]->last, p1[i]->phone``` (Note this is a person struct array) – CardBoardBox Feb 16 '20 at 04:37
  • If it is an array-of-struct, then the `[..]` acts as a *dereference* as well, just as the `->` operator does, so you will need `p1[i].first`, etc.. (when you pass an array, it is actually passed as a pointer to the first element, so when you use indexing with `[..]` you have already dereferenced the pointer leaving type `struct Person` instead of a pointer to it, so you use the `'.'` operator) – David C. Rankin Feb 16 '20 at 04:41
  • Here is how I currently have my fprintf... ```fprintf(file_out, "%s, %s, %s\n", p1[i].first, p1[i].last, p1[i].phone);``` and I am getting there error.... Error: '*(p1 + (sizetype)((long unsigned int) i * 8))' is a pointer, did you mean to use '->'? – CardBoardBox Feb 16 '20 at 04:42
  • If you are getting that error, then you declared an *Array of Pointers to Struct* instead of an *Array of Struct* -- two separate and distinct types. The *Array of Pointers* would be declare as `struct Person *myarray[size];` the *Array of Struct* would be declared as `struct Person myarray[size];` (no `'*'` before `myarray`) -- Which did you do? If you edit your question and include your complete code (don't delete any of your original question, just **ADD** the edit at the end) -- I can tell exactly what is going on `:)` – David C. Rankin Feb 16 '20 at 04:52
  • Just commented under your Question, choose one or the other `'*'` or `[..]`, but not both for your `p1` parameter in `void print(FILE* file_out, struct Person* p1[]){` I would simply use `void print(FILE *file_out, struct Person *p1){` PS, the `'*'` goes with the variable, not the type. Why? `char* a, b, c;` does NOT declare 3-pointers to `char`, it declares one pointer `a` and two normal characters `b, c`. Using `char *a, b, c;` makes that clear. – David C. Rankin Feb 16 '20 at 05:09