Need help with improving my programming

**Mikro** · 01-03-2003

I need help with improving and optimizing my program. My original plan was to create a small man2html program but it seems now that is more like a general txt2html.

I am trying to get my code so close to ANSI-C standard as possible.

Code:

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

#define MAX 10000
#define LITE 100

void str2html();

char words[MAX];

FILE * fp;
FILE * st;

int main()
{
	char readFile[LITE];
	char writeFile[LITE];

	printf("Enter file to open\n");
	scanf("%s", &readFile);

	printf("Enter file to write to\n");
	scanf("%s", &writeFile);

	fp = fopen(writeFile, "w");
    st = fopen(readFile, "r");

	fflush(stdin);

	fprintf(fp, "<html>\n<head>\n<body bgcolor=""white"">\n</head>\n");
    
    while (fgets(words, MAX, st) != NULL && words[0] != '\0')
		fprintf(fp, "%s <br>", words);
	
	fprintf(fp, "\n</body>\n</html>");
	str2html();

	fclose(st);
	fclose(fp);

    return 0;
}

void str2html()
{
	int val;
	char str[LITE];
	char *pdest;

	printf("Do you want to make html links? 1 for yes, 2 for no\n");

	scanf("%d", &val);

	while (val == 1)
	{
		printf("Enter word to make a link to\n");
		scanf("%s", &str);
		pdest = strstr(words, str);

		if (pdest != NULL)
		{
			char ch[100];
			printf("Enter shortcut to the word\n");
			scanf("%s", &ch);
			fprintf(fp, "<a href=%s%s.html>", ch, str);
			fprintf(fp, "%s", str);
			fprintf(fp, "</a> ");
		}
		else
			printf("Error\n");

		printf("Do you want to continue? 1 for yes, 2 for no\n");
		scanf("%d", &val);
	}
	printf("Exit\n");
}

Thanks in advance.

**quzah** · 01-03-2003

Code:

fflush(stdin);

Flushing instreams is undefined behaviour. (IE: Bad.) You should use something else instead:

while((c=getchar( ))!='\n');

Search the board for the topic if you want to know more.

Quzah.

**WolfPack** · 01-03-2003

I didn't look all the way through it as I am working on a program myself but on line 26 and 27 you always want to make sure that the two files were actually created, so add some error checking for that. I'll look through the full thing later tonight.

**Shiro** · 01-03-2003

It would be more elegant to remove the global variables, words, fp and st to the main() function and pass them as parameters to the function str2html().

You should check the result of fopen().

Code:

if (NULL == fp || NULL == st)
{
    /* one or both files not opened */
}

**wadim** · 01-03-2003

Hi,

you deklare readFile writeFile and str as char arrays but i think you use the scanf function wrong while reading them.

instead:

scanf("%s", &readFile)

it should be:

scanf("%s", readFile)

without &

**Prelude** · 01-03-2003

>I am trying to get my code so close to ANSI-C standard as possible.

>void str2html();
Not ANSI C. This is an old K&R style function declaration that simply says "This function takes an unknown amount of arguments". Since you're going for ANSI compliance, let's change this to

void str2html ( void );

>void str2html();
Here again? Yes. This name walks perilously close to invading the implementation's reserved namespace. The exact words of the standard are "Function names that begin with str, mem, or wcs and a lowercase letter may be added to the declarations in the <string.h> header." The number 2 isn't considered a lower case letter, but as a matter of good form you should avoid any identifiers that begin with str. So let's change this function yet again:

void stohtml ( void );

>int main()
This really should be

int main ( void )

though in the case of main I usually make an exception in my pedanticism.

>scanf("%s", &readFile);
There's no need for & when your argument is already a pointer.

>scanf("%s", &writeFile);
Ditto

>fp = fopen(writeFile, "w");
>st = fopen(readFile, "r");
Check to make sure that these calls to fopen worked, if not fp or st will be NULL and trying to use a NULL pointer is a Bad Thing.

>fflush(stdin);
fflush is only defined for output streams by the standard, some compilers support input streams as well, but that doesn't make it correct.

>while (fgets(words, MAX, st) != NULL && words[0] != '\0')
fgets reads the trailing newline as well, so if your file happens to have lines like so:

Line 1\n
Line 2\n
\n
Line 3\n
<eof>

Then if words is not NULL, words[0] will never be '\0'. A better test would check for '\n' instead.

>scanf("%d", &val);
This stands for the above calls as well, always check the return value of scanf.

>printf("Error\n");
A better error report would use fprintf and stderr.

-Prelude

**Mikro** · 01-04-2003

I have now made some changes.

New code.

Code:

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

#define MAX 10000
#define LITE 100

void str2html(FILE * fp);

char words[MAX];

int main()
{
	FILE * fp;
	FILE * st;
	char readFile[LITE];
	char writeFile[LITE];

	printf("Enter file to read\n");
	scanf("%s", readFile);

	printf("Enter file to write to\n");
	scanf("%s", writeFile);

	if ((fp = fopen(writeFile, "w")) == NULL)
	{
		fprintf(stderr, "Can't write to file!", writeFile);
		exit(1);
	}

    if ((st = fopen(readFile, "r")) == NULL)
	{
		fprintf(stderr, "Can't read file!", readFile);
		exit(2);
	}

	fprintf(fp, "<html>\n<head>\n<body bgcolor=""white"">\n</head>\n");
    
    while (fgets(words, MAX, st) != NULL && words[0] != '\0')
		fprintf(fp, "%s <br>", words);
	
	fprintf(fp, "\n</body>\n</html>");
	str2html(fp);

	fclose(st);
	fclose(fp);

    return 0;
}

void str2html(FILE * fp)
{
	int val;
	char str[LITE];
	char *pdest;

	printf("Do you want to make html links? 1 for yes, 2 for no\n");
	scanf("%d", &val);

	if (val < 1 || val > 2)
	{
		printf("Enter a value 1 or 2\n");
		scanf("%d", &val);
	}

	while (val == 1)
	{
		printf("Enter word to make a link to\n");
		scanf("%s", &str);
		pdest = strstr(words, str);

		if (pdest != NULL)
		{
			char ch[100];
			printf("Enter shortcut to the word\n");
			scanf("%s", &ch);
			fprintf(fp, "<a href=%s%s.html>", ch, str);
			fprintf(fp, "%s", str);
			fprintf(fp, "</a> ");
		}
		else
			fprintf(stderr, "Error, can't find word\n");

		printf("Do you want to continue? 1 for yes, 2 for no\n");
		scanf("%d", &val);
		
		if (val < 1 || val > 2)
		{
			printf("Enter a value 1 or 2\n");
			scanf("%d", &val);
		}
	}
	printf("End of program\n");
}

Prelude:
/*
>while (fgets(words, MAX, st) != NULL && words[0] != '\0')
fgets reads the trailing newline as well, so if your file happens to have lines like so:

Line 1\n
Line 2\n
\n
Line 3\n
<eof>

Then if words is not NULL, words[0] will never be '\0'. A better test would check for '\n' instead.
*/

I have tried with '\n' but is doesn¨t behave completly the way as expected, for me is better with '\0'.

I have tested with GCC 3.2 under Windows 2000sp3 and Red Hat 7.3

**rmullen3** · 01-04-2003

Why not both '\0' and '\n'? =P

Thread: Need help with improving my programming

Thread Tools

Search Thread

Display

Need help with improving my programming

~

Similar Threads

Improving the efficiency..

Improving Code Interface

Improving my code

Code improving