Need help with improving my programming

This is a discussion on Need help with improving my programming within the C Programming forums, part of the General Programming Boards category; I need help with improving and optimizing my program. My original plan was to create a small man2html program but ...

  1. #1
    Registered User
    Join Date
    Feb 2002
    Posts
    37

    Need help with improving my programming

    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.

  2. #2
    ATH0 quzah's Avatar
    Join Date
    Oct 2001
    Posts
    14,826
    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.
    Hope is the first step on the road to disappointment.

  3. #3
    Registered User
    Join Date
    Nov 2002
    Posts
    29
    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.

  4. #4
    ....
    Join Date
    Aug 2001
    Location
    Groningen (NL)
    Posts
    2,386
    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 */
    }

  5. #5
    Registered User
    Join Date
    Jan 2003
    Posts
    2
    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 &

  6. #6
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,796
    >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
    My best code is written with the delete key.

  7. #7
    Registered User
    Join Date
    Feb 2002
    Posts
    37
    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

  8. #8
    Registered User rmullen3's Avatar
    Join Date
    Nov 2001
    Posts
    330

    ~

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

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Improving the efficiency..
    By aaronljx in forum C Programming
    Replies: 11
    Last Post: 12-01-2008, 09:09 AM
  2. Improving Code Interface
    By helloamuro in forum C Programming
    Replies: 20
    Last Post: 05-02-2008, 04:34 AM
  3. Improving my code
    By rwmarsh in forum C++ Programming
    Replies: 14
    Last Post: 07-08-2006, 11:18 AM
  4. Code improving
    By Ideswa in forum Game Programming
    Replies: 6
    Last Post: 04-06-2006, 09:49 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21