sending a struct to a function

Hi Guys,

I have the following code that is working fine and I was wondering what is the preffered way to send an array of struct to a function, and if I should make any changes to make this code look more proffestional.

Code:

---

#include <stdio.h>
#include <stdlib.h>
#include <conio.h>


typedef struct
{
  int day,month,year;
} Date;


typedef struct
{
  Date date;
  int hh,mm;
  char description[100];

} Meetings;


void ShowMenu()
{
  system("CLS");
  puts("a. Add");
  puts("d. Delete");
  puts("u. Update");
  puts("s. Show All");
  puts("x. Exit");
}

void AddMeeting(Meetings *m)
{
  puts("Date:");
  scanf("%d/%d/%d",&m->date.day, &m->date.month, &m->date.year);
  puts("Hour:");
  scanf("%d:%d",&m->hh, &m->mm);
  flushall();
  puts("Description:");
  gets(m->description);
}

Meetings *AllocateMemory(Meetings *m, int *iTotal)
{
  ++(*iTotal);
  m=realloc(m,(*iTotal)*sizeof(*m));
  if (m==NULL)
  {
        puts("Not enough memory!");
        exit(1);
  }
  return m;
}

int main(void)
{
  Meetings *meetings=NULL; 
  int iTotal=0;
  char ch;
 
  do
  {
    ShowMenu();
        ch=getch();

        switch(ch)
        {
          case 'a':
                meetings=AllocateMemory(meetings,&iTotal);
                AddMeeting(&meetings[iTotal-1]);
          break;
        }

  } while (ch!='x');

  if (meetings!=NULL)
        free(meetings);

  return 0;
}

---

Many Thanks!!!

> proffestional
Spelling "professional" itself properly goes a long way. :)

The only thing I'd say is that structures of any reasonably large size are generally passed by reference (with pointers) rather than by value, to avoid clobbering the stack (in other words, avoid taking lots of memory). You're already doing this, though, so no worries there.

However, there's lots of other things wrong with your code.

• gets() is bad, use fgets(). See the FAQ.
• fflush(stdin) is bad. See the FAQ.
• system("CLS") is not very good, either. See the FAQ.

Consider not using the non-standard conio.h getch() as well. See the FAQ. :p

"Where is this FAQ you speak of?" . . . http://faq.cprogramming.com/

Also, free(NULL) is guaranteed to do nothing, so this

Code:

---

  if (meetings!=NULL)
        free(meetings);

---

is redundant.

Excellent words DWK!

I have one more question.
Which one of the following is the best way to pass the first element of the struct to a function?

Code:

---

1. AddMeeting(&meetings[0]);
2. AddMeeting(meetings);
3. AddMeeting(&meetings);

---

Thanks Again!

Well, the first two rely on a function taking a type* or type[] parameter, which is a good thing most of the time. The third one makes you take a parameter like type**, which, unless you're going to change where the pointer actually points to (rather rare), is just extra syntax.

As for passing an array to a function -- I prefer array to &array[0]. It's shorter, and it sort of indicates to the reader that you're dealing with the whole array, not the first element of the array.

Thank you again DWK!
Good spelling and programming lesson!

Quote:

---

Originally Posted by salvadoravi

Excellent words DWK!

I have one more question.
Which one of the following is the best way to pass the first element of the struct to a function?

Code:

---

1. AddMeeting(&meetings[0]);
2. AddMeeting(meetings);
3. AddMeeting(&meetings);

---

Thanks Again!

---

The third is not equivalent to the first two. Of the first two, I would usually use the second. (If I saw the first in code, I would either think that (a) the person didn't know that meetings == &meetings[0], or (b) the person is explicitly stating that s/he only intends to deal with the first element of the array, depending on how competent the rest of the code looks and the comments nearby.) If you intend your function to process the whole array, you should certainly use form #2. I would only use form #1 if I really wanted to emphasize that I was only looking at element #0, or for parallelism with something like AddMeeting(&meetings[iTotal-1]) where the address-of-element format is clearly best.

Either way, if your comments tell (accurately!) what's going on, I don't think anyone will care much about 1 vs. 2.

Oh, you're welcome. :)

A few other things:

Code:

---

  puts("Date:");
  scanf("%d/%d/%d",&m->date.day, &m->date.month, &m->date.year);

---

You should specify what format you're expecting the date to be in -- dates can be written in many different ways. You might also consider using %*c instead of /'s in your scanf() format string -- %*c skips any character, so -'s or /'s or whatever would work.

Code:

---

m=realloc(m,(*iTotal)*sizeof(*m));

---

That is generally a bad idea. If realloc() fails, it returns NULL, and you've lost your original block of memory that m was pointing to. It probably doesn't really matter in this case, but it's something you should keep in mind.

In other words, don't use p=realloc(p,...) -- use a temporary variable, so that you can free p.

Code:

---

char *temp = realloc(p, ...);
if(!temp) {
    free(p);
    perror("Out of memory");
    exit(1);
}

---

Oh, and checking for NULL from realloc() and malloc() is a good idea in any case.

Notice perror() -- it can be quite useful at times. http://www.opengroup.org/onlinepubs/...ns/perror.html

Excellent use of sizeof(*m), by the way. :) But you have to obfuscate it!

Code:

---

*iTotal*sizeof*m

---

Just kidding. ;)

Your indentation is reasonably good, although it could be more consistent. It wouldn't pass an Elysia examination. :p

Quote:

---

Originally Posted by tabstop

If you intend your function to process the whole array, you should certainly use form #2. I would only use form #1 if I really wanted to emphasize that I was only looking at element #0, or for parallelism with something like AddMeeting(&meetings[iTotal-1]) where the address-of-element format is clearly best.

---

What will be the preffered way:

Code:

---

1. AddMeeting(&meetings[iTotal-1])
2. AddMeeting(&meetings+iTotal-1)

---

Quote:

---

Originally Posted by dwks

Oh, you're welcome. :)
Excellent use of sizeof(*m), by the way. :) But you have to obfuscate it!

Code:

---

*iTotal*sizeof*m

---

Just kidding. ;)

---

I don't understand should I change sizeof(*m) to sizeof*m ??

Thanks again

Quote:

---

Originally Posted by salvadoravi

What will be the preffered way:

Code:

---

1. AddMeeting(&meetings[iTotal-1])
2. AddMeeting(&meetings+iTotal-1)

---

---

Number 2 would have to be AddMeeting(meetings+iTotal-1); remember that meetings by itself is already a pointer, so &meetings is a pointer to a pointer and then it all gets weird.

As to which is preferred: anyone who voluntarily uses #2 over #1 should be shot. :) (Well, maybe not really.)

Quote:

---

Originally Posted by salvadoravi

I don't understand should I change sizeof(*m) to sizeof*m ??

Thanks again

---

No. Obfuscated can be interesting when you're in the mood for it, but for actual real code with actual real programs, no.

Quote:

---

Originally Posted by dwks

Also, free(NULL) is guaranteed to do nothing, so this

Code:

---

  if (meetings!=NULL)
        free(meetings);

---

is redundant.

---

That depends on how old the compiler / C runtime is. Many older compilers will not "ignore NULL" when called for free, which of course will mess up the entire memory allocation ever after. That said, it is a fair argument that if this doesn't work right, it's a good reason to consider ther compiler "ancient and in need of replacement with something new", just like a car with 500000 miles on the clock wouldn't be a good thing.

--
Mats

Yeah, allowing free(NULL) is part of C89, so any compiler that doesn't allow it must be very old indeed. http://www.cygwin.com/ml/gdb-patches.../msg00215.html

Quote:

---

Originally Posted by dwks

Your indentation is reasonably good, although it could be more consistent. It wouldn't pass an Elysia examination. :p

---

You're right :p
Your indentation is good, but it could be better! Good indentation is a good thing.
Try not to mix spaces and tabs - it usually messes up code outside your editor! So use only tabs or only spaces. I recommend tabs.
Also make the indentation consistent. Don't indent two spaces somewhere and a tab somewhere else. Just use one tab everywhere and everyone will be happy!

Quote:

---

Originally Posted by matsp

That depends on how old the compiler / C runtime is. Many older compilers will not "ignore NULL" when called for free, which of course will mess up the entire memory allocation ever after. That said, it is a fair argument that if this doesn't work right, it's a good reason to consider ther compiler "ancient and in need of replacement with something new", just like a car with 500000 miles on the clock wouldn't be a good thing.

--
Mats

---

What is your recommendation?
Should I use

Code:

---

if (meetings!=NULL)
  free(meetings);

---

For portability?

Or should I assume that anyone have new compiler?

Quote:

---

Originally Posted by draft c99

The free function causes the space pointed to by ptr to be deallocated, that is, made
available for further allocation. If ptr is a null pointer, no action occurs. Otherwise, if
the argument does not match a pointer earlier returned by the calloc, malloc,or
realloc function, or if the space has been deallocated by a call to free or realloc,
the behavior is undefined.

---

So long as your compiler is sporting a nice shiny "ANSI" badge, then all should be well.

> Or should I assume that anyone have new compiler?
That depends how much effort you want to put into it. Some code goes out of it's way to be compatible with as many things as possible, but it can look horrible if managed poorly.

Unless there is a need (or anticipation) to run your code on pre-ANSI C, then freeing NULL will be the least of your problems. The biggie to fix would be that pre-ANSI C doesn't understand function prototypes.

Edit: I see dwks already mentioned C89 :)