Thank you all for reply,
I did my assignment before I got the first reply but anyway I learned
more from you guys. The only problem I had when my teacher check the
assignment is memory leak. I don't know where I made mistake
/**************** Code ********************/
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "a.h" // defination of some constants and message
struct
Struct what?
(This is why C++ comments are bad on Usenet)
Also you should post the contents of a.h
/************ FUNCTION PROTOTYPES **************/
void checkPtr(void *p);
void performAction(char *cmd);
void printList();
void add();
void deleteMsg();
int find(int id);
int sbi(const void *a,const void *b);
int sbt(const void *a,const void *b);
void freeAll();
message *list;
int currentSize;
int counter = 0;
Global variables, brrr. It would be better to put these three
in a struct at least (since they are all related to the same
conceptual object).
An expert programmer would declare this in main() and pass
it to whatever functions need it.
This means that you could have another list within the same
program, if you wanted, and it also makes it very clear which
parts of the program are affected if you change the structure
of the list. (This principle is called 'encapsulation').
However this makes freeing it slightly more complicated
(you can't just have a freeAll() function like you do now),
so you can hold off on this improvement until you've sorted
out all the other problems.
char *cmdBuffer; // for reading command lines
This could be declared (and freed) within main().
int main()
{
currentSize = INITIAL_CAPACITY;
list = (message *)calloc(INITIAL_CAPACITY,sizeof(message));
It looks like your instructor didn't teach malloc very well:
list = malloc(INITIAL_CAPACITY * sizeof *list);
It is pointless to use 'calloc' if you're allocating pointers
(it doesn't necessarily set them to NULL). If you actually want
all the members of 'list' to start off as NULL then you'll have
to write a loop to go through and set them one by one.
(Looking at your code, I think it works fine if they are not
NULLed).
checkPtr(list);
cmdBuffer = (char *) malloc(MAX_CMD_LEN);
cmdBuffer = malloc(MAX_CMD_LEN);
Casting the return value of malloc gains you absolutely nothing,
and in some cases it can cause trouble. (Read the FAQ for more info).
Also, since MAX_CMD_LEN changes then there is no need to use
dynamic allocation at all -- it is merely a source of errors.
Instead, do:
char cmdBuffer[MAX_CMD_LEN];
at the start of main(). Then you don't have to check a pointer
and you don't have to free it.
checkPtr(cmdBuffer);
while((scanf("%s",cmdBuffer)) != EOF)
{
performAction(cmdBuffer);
}
freeAll();
return 0;
}
void performAction(char *cmd)
{
int f,fid;
if(strcmp(cmd,"add")==0)
add();
else if(strcmp(cmd,"delete")==0)
deleteMsg();
else if(strcmp(cmd,"find")==0)
{
scanf("%d",&fid);
You should check this scanf() call for failure, like you did
with the previous scanf.
f = find(fid);
if(f != -1)
{
printf("%s\n",((list + f))->messageText);
}
}
else if(strcmp(cmd,"output")==0)
printList();
else if(strcmp(cmd,"sortById")==0)
{
qsort(list,counter,sizeof(message),sbi);
As others have suggested: sizeof *list.
Then if you ever change list to something other than "message *"
you don't have to go through and change all your sizeof's.
}
else if(strcmp(cmd,"sortByText")==0)
qsort(list,counter,sizeof(message),sbt); > }
int sbi(const void *a,const void *b)
{
return (((message *)a)->messageId - ((message *)b)->messageId);
}
int sbt(const void *a,const void *b)
{
const char *cs = (const char *)(((message *)a)->messageText);
const char *ct = (const char *)(((message *)b)->messageText);
These variables are unnecessary. The casts are unnecessary,
unless messageText is not a char*, which I don't know since you
didn't include the text of "a.h".
"return" doesn't need brackets. (It's not a function).
return strcmp( ((message *)a)->messageText,
((message *)b)->messageText );
}
void deleteMsg()
{
int id,i,index;
char *tmpTxt;
message *tmp;
scanf("%d",&id);
Again, check the return value of scanf
index = find(id);
//if message not found
if(index == -1)
return;
//printf("Deleting\n");
tmp = (list + index); //list[index]
tmpTxt = tmp->messageText;
free(tmpTxt);
//free(tmp);
Right, you don't need to free tmp, because the whole of
'list' is allocated as one chunk.
So you could have saved some effort with:
free( list[index].messageText );
instead of all that stuff from "Deleting\n" onwards.
It would probably be better design to have a function
for freeing a message, rather than freeing messageText
all over the place. (Then if you added another string to
the message structure, you would only have to add the
free() call in one place).
counter--;
for(i = index;i < counter;i++)
{
*(list + i) = *(list + (i+1));
Or just: list
= list[i+1]
}
}
int find(int id)
{
message *tmp;
int i;
for(i = 0; i < counter; i++)
{
tmp = (list + i);
if((tmp->messageId) == id)
return i;
Again you don't need this temp variable:
if (list.messageId == id)
would have been just fine.
}
return -1;
}
void add()
{
int id,len;
int i;
char *txt;
char *str;
//string withour newline character and exact length
message *msg;
message *tmp;
scanf("%d\n",&id); //reads msg id
Check scanf for failure.
txt = (char *) malloc(MAX_TEXT_LEN);
Lose the cast. But as I mentioned before, it would be better
to not use dynamic allocation at all here:
char txt[MAX_TEXT_LEN];
That reduces the chance of memory leaks.
checkPtr(txt);
fgets(txt,MAX_TEXT_LEN,stdin);
Check fgets for failure.
// skip if find the msg with same id
if(find(id) != -1)
{
free(txt);
return;
}
You should do that before you input the txt, perhaps.
/***** If the list of pointer is full then allocate more space ****/
if(counter == currentSize)
{
currentSize += CAPACITY_INCREMENT;
tmp = (message *)realloc((void *)list,currentSize *
sizeof(message));
checkPtr(tmp);
tmp = realloc(list, currentSize * sizeof *list);
list = tmp;
}
len = strlen(txt); //length of string
Length of string, really? (avoid useless comments)
str = (char *) malloc(len);
checkPtr(str);
*(txt+(len-1)) = '\0';
strcpy(str,txt);
free(txt);
This would be a memory leak if len is 0. Maybe that is
what your instructor found.
Also your code is a little non-idiomatic (by which I mean, I
thought it was wrong at first). This code is easier to follow:
txt[--len] = '\0';
str = malloc(len + 1);
checkPtr(str);
strcpy(str, txt);
because 'len' reflects the actual length of the string, at all times.
msg = (list + counter++);
msg->messageId = id;
msg->messageText = str;
}
void printList()
{
message *tmp;
int i;
for(i = 0; i < counter; i++)
{
tmp = (list + i);
//printf("%d\n",tmp->messageId);
printf("%s\n",tmp->messageText);
}
}
void freeAll()
{
message *tmp;
char *str;
int i;
for(i = 0; i < counter; i++)
{
tmp = (list + i);
free(tmp->messageText);
}
free(list);
free(cmdBuffer);
}
Same again, you could have written those two without 'tmp'