PDA

View Full Version : [C] Need Some Input on My Code



loganwm
September 21st, 2008, 03:12 AM
I'm working on a program that is (for lack of a better term) a "dumb ai."

It basically will convert typed input into crude commands into a small interpreter I'm writing.

Right now I've designed a tiny basis for the interpreter and some basic functions for the short-term memory.

Any input on my organization and any issues is appreciated.

I've also add a small Makefile, just type "make default" to compile the way I've set it up.


I've only documented some scant pieces showing implemented functions in main, but I'll probably do the rest soon.

I'm also adding some real commands, but I just wanted to get this code up and have some "peer editing" of sorts and constructive criticism.

scourge
September 21st, 2008, 05:28 PM
- main() should return an int
- always turn compiler warnings to the max
- use the "extern" keyword for declaring external functions
- if you can remove an #include from a header file and put it in the source file instead, you often should
- Why does Command() take a struct node* parameter, but doesn't do anything with it?
- You're using strncmp() to compare only one character, why?

Here's a patch for you, so you can see what I was aiming for:



diff -u a/main.c b/main.c
--- a/main.c 2008-09-21 04:29:37.000000000 +0300
+++ b/main.c 2008-09-21 19:11:07.000000000 +0300
@@ -6,9 +6,9 @@

#include <stdlib.h>
#include <stdio.h>
-#include <string.h>
#include "text.h"
#include "memory.h"
+#include "process.h"



@@ -25,6 +25,7 @@
inputArr = GetWordsFromString(input,BASIC_DELIMS); //format string using delims to string array
Command(inputArr,root); //
}
+
+ return EXIT_SUCCESS;
}

-
diff -u a/Makefile b/Makefile
--- a/Makefile 2008-09-21 04:28:17.000000000 +0300
+++ b/Makefile 2008-09-21 19:03:58.000000000 +0300
@@ -1,4 +1,5 @@
-cc = gcc
+CC = gcc

make default:
- $(cc) main.c process.c text.c memory.c -o program
+ $(CC) -Wall -pedantic -std=gnu99 main.c process.c text.c memory.c -o program
+
diff -u a/memory.c b/memory.c
--- a/memory.c 2008-09-20 17:48:57.000000000 +0300
+++ b/memory.c 2008-09-21 19:14:02.000000000 +0300
@@ -1,3 +1,5 @@
+#include <stdlib.h>
+#include <string.h>
#include "memory.h"

struct node *AddNodeToEnd(struct node *root_, char *name_, char *owner_, char *value_) {
@@ -23,7 +25,7 @@

struct node *FindNodeByName(struct node *root_, char *name_) {
struct node *traverse;
- char nameKey[32];
+ //char nameKey[32];
traverse = root_;

while (traverse->next != NULL) {
diff -u a/memory.h b/memory.h
--- a/memory.h 2008-09-20 17:44:54.000000000 +0300
+++ b/memory.h 2008-09-21 19:07:43.000000000 +0300
@@ -1,6 +1,5 @@
-#include <stdlib.h>
-#include <stdio.h>
-
+#ifndef MEMORY_H
+#define MEMORY_H

struct node {
char* name;
@@ -9,6 +8,9 @@
struct node *next;
};

-struct node *AddNodeToEnd(struct node*, char*, char*, char*);
+extern struct node *AddNodeToEnd(struct node*, char*, char*, char*);
+
+extern struct node *FindNodeByName(struct node*, char*);
+
+#endif // MEMORY_H

-struct node *FindNodeByName(struct node*, char*);
diff -u a/process.c b/process.c
--- a/process.c 2008-09-21 04:15:49.000000000 +0300
+++ b/process.c 2008-09-21 19:12:15.000000000 +0300
@@ -1,3 +1,6 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
#include "process.h"

void Command(char** string, struct node* root) {
diff -u a/process.h b/process.h
--- a/process.h 2008-09-21 00:00:30.000000000 +0300
+++ b/process.h 2008-09-21 19:07:20.000000000 +0300
@@ -1,7 +1,9 @@
-#include <stdlib.h>
-#include <stdio.h>
-#include <string.h>
+#ifndef PROCESS_H
+#define PROCESS_H

-#include "memory.h"
+struct node;
+
+extern void Command(char**, struct node*);
+
+#endif // PROCESS_H

-void Command(char**, struct node*);
diff -u a/text.c b/text.c
--- a/text.c 2008-09-19 05:11:20.000000000 +0300
+++ b/text.c 2008-09-21 19:12:49.000000000 +0300
@@ -1,3 +1,5 @@
+#include <stdlib.h>
+#include <string.h>
#include "text.h"

char **GetWordsFromString(char* string, char* delims) {
diff -u a/text.h b/text.h
--- a/text.h 2008-09-21 04:26:15.000000000 +0300
+++ b/text.h 2008-09-21 19:08:38.000000000 +0300
@@ -1,7 +1,9 @@
-#include <stdlib.h>
-#include <string.h>
+#ifndef TEXT_H
+#define TEXT_H

#define BASIC_DELIMS " ,'\n"

-char **GetWordsFromString(char*, char*);
+extern char **GetWordsFromString(char*, char*);
+
+#endif // TEXT_H

cb951303
September 21st, 2008, 05:40 PM
First thing I noticed: you need to protect your headers from being included more than one time.

add something like this to all your header files



#ifndef HEADER_FILE_NAME_H //You should use a unique name here such as the name of the header file
#define HEADER_FILE_NAME_H

/* Header file contents goes here */

#endif

EDIT1: I see you use a lot of malloc() but not one free()
that's a bad practice. even though your application doesn't need any memory freeing right now because it terminates before you have to free them, in future you may regret this as it will get harder to keep track of allocated memory blocks since C doesn't have garbage collection. My advice is free always all the the allocated memories even though you don't need it.

EDIT2: memory.c and memory.h are not used?

EDIT3: concerning EDIT1: actually you have a memory leak
inputArr = GetWordsFromString(input,BASIC_DELIMS); and it took me 2-3 minutes to notice it. now imagine you have hundreds lines of code ... :)

loganwm
September 21st, 2008, 06:02 PM
I'll probably set to work tearing this apart and fixing the horrible organization and such.

So, what you're saying is adding that line at the beginning of the header allows it to only be included once? and What about a standard header, like sdtlib.h and stdio.h, should a similar trick be used?


And can you help me spot that memory leak? Or at least describe it?

Sockerdrickan
September 21st, 2008, 06:10 PM
And can you help me spot that memory leak? Or at least describe it?
http://bootloop.blogspot.com/2008/09/c-memory-deallocation.html :)

cb951303
September 21st, 2008, 06:16 PM
I'll probably set to work tearing this apart and fixing the horrible organization and such.

So, what you're saying is adding that line at the beginning of the header allows it to only be included once? and What about a standard header, like sdtlib.h and stdio.h, should a similar trick be used?


And can you help me spot that memory leak? Or at least describe it?

standard libraries already have that protection. It's a very simple conditional protection actually. If that string is already defined, compiler skips whatever stated between "#ifndef" and "#endif" in other words your header file is not included.

in the function "GetWordsFromString(input,BASIC_DELIMS)" you allocate memory for the variable "output". Than the address of that memory block is passed to "inputArr". But every time the loop iterates the function "GetWordsFromString(input,BASIC_DELIMS)" is recalled thus it creates a new memory block. Then its address is passed to "inputArr". So you lose the old memory address meaning you can't free that block never since you don't know its address. What you should do is to add "free(inputArr);" to the end of the for loop.

monkeyking
September 21st, 2008, 11:49 PM
And can you help me spot that memory leak? Or at least describe it?

You can use 2 programs for testing the memory correctness of your program.
Run your program through gdb, or run it through valgrind.

good luck