Like Tree1Likes

function help

This is a discussion on function help within the C Programming forums, part of the General Programming Boards category; Hi, I'm following the K&R book and I'm on excersise 3-3 creating an expand function. Here's what I have so ...

  1. #1
    Registered User
    Join Date
    Nov 2010
    Posts
    45

    function help

    Hi, I'm following the K&R book and I'm on excersise 3-3 creating an expand function.
    Here's what I have so far:
    Code:
    // my_expand.c
    /* program to take a given expression and parse it
     * into the entire range. e.g. a-z = abcdef...xyz.
     */
     
     #include <string.h>
     #include <stdio.h>
     #include <stdlib.h>
     #include <math.h>
     
     int check_validation(char *pointer);     // validate the input
     char *expand(char *s1, char *s2);        // prototype of function (expand())
     
     int main(int argc, char **argv)
     {
    	if ((argc < 2) || (argc > 2))
    	{
    		fprintf(stderr, "Usage: %s <expression> e.g. a-z, 1-9 or A-Z0-9\n", argv[0]);
    		return -1;          // check that the right num of args are there
    	}
    	
    	char result[40];
    	
    	if (check_validation(argv[1]) != 1)
    	{
    		fprintf(stderr, "Usage: %s <expression> e.g. a-z, 1-9 or A-Z0-9\n", argv[0]);
    		return -1;
    	} else
    	{
    		printf("The expression used: %s\n", argv[1]);
    		printf("And the result: %s\n", expand(argv[1], result));
    		return 0;
    	}
    }
    
    int check_validation(char *pointer)    // function to validate input
    {
    	if (((strlen(pointer)) != 3) || (strlen(pointer) != 6))
    		return -1;
    	
    	int i, j;
    	for (i = 0; i < strlen(pointer); i++)
    	{
    		if ((i == 0 || i == 2) || (i == 3 || i == 5))
    		{
    			if ((i > 0) && (i != 3))
    			{
    				if ((pointer[i] - pointer[i-2]) > (('z' - 'a') || ('9' - '0')))
    					return -1;
    				else if ((pointer[i] - pointer[i-2] <= 0))
    					return -1;
    			}
    			else if ((pointer[i] >= 'a') && (pointer[i] <= 'z'))
    				break;
    			else if ((pointer[i] >= 'A') && (pointer[i] <= 'Z'))
    				break;
    			else if ((pointer[i] >= '0') && (pointer[i] <= '9'))
    				break;
    			else
    				return -1;
    		} else if (i == 1 || i == 4)
    		{
    			switch(pointer[i])
    			{
    				case '-':
    					break;
    				default:
    					return -1;
    					break;
    			}
    		}
    	}
    	return 1;
    }
    
    char *expand(char *s1, char *s2)     // write the expand function to return result
    {
    	int i = 0;
    	while (s1[i] != s1[i+2])
    		s2[i] = *s1[i++];
    	i = s1[i+2] - s1[i];
    	if (strlen(s1) == 6)
    	{
    		int j = 3;
    		while (s1[j] != s1[j+2])
    			s2[i] = *s1[i++];
    	}
    	return s2;
    }
    And in the expand() function:
    Code:
    char *expand(char *s1, char *s2)     // write the expand function to return result
    {
    	int i = 0;
    	while (s1[i] != s1[i+2])
    		s2[i] = *s1[i++];
    	i = s1[i+2] - s1[i];
    	if (strlen(s1) == 6)
    	{
    		int j = 3;
    		while (s1[j] != s1[j+2])
    			s2[i] = *s1[i++];
    	}
    	return s2;
    }
    say the user types "my_expand a-z" in CMD, then s1[i] should be 'a'.
    Is it possible to increment the constant 'a' without incrementing the string s1 so
    the next value would be 'b' instead of '-' in the string a-z??

    Thanks.

  2. #2
    Banned
    Join Date
    Aug 2010
    Location
    Ontario Canada
    Posts
    9,547
    Ok, pardon the direct approach here but you've got more problems than just incrementing values in an array. The function call to expand is going to fail you miserably... If it works at all it will work by strength of dumb luck only. You would be better to not return a value from that function and then print the value from result array in your main... C does not know now to to return an array from a function. The only reason it's working at all is that you are passing in a pointer so that you are actually modifying the variable result[40] in your main function.

    Before you do much else you need to get that sorted out.

    The actual range expansion is fairly simple. It should require no more than a simple loop counting from the start value to the end value, incrementing an array index as it goes... about 5 lines of code.

  3. #3
    Programming Wraith GReaper's Avatar
    Join Date
    Apr 2009
    Location
    Greece
    Posts
    1,625
    Assuming that you want "a-z" to expand to "abcdefghijklmnopqrstuvwxyz"
    Code:
    int i, j;
    ...
    for (i = 'a', j = 0; i <= 'z'; i++, j++){
        s2[j] = (char)i;
    }
    
    s2[j] = '\0';
    
    return s2;
    Last edited by GReaper; 06-13-2011 at 04:22 PM.
    Devoted my life to programming...

  4. #4
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    Ok, thanks for the help.

  5. #5
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    In addition, could anyone point out any other poorly written code other than what commontater pointed out so i can learn from that.
    It would be most appreciated.

  6. #6
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,629
    These are the things I think you could improve on. I'm sure there's more, but this is what I got at a first glance:

    You correctly declared main to return an int, but you should avoid returning negative numbers. I know for a fact in Linux that returning -1 from main gets interpreted by the shell as a return value of 255. The shell expects back a unsigned char (0-255), so the 0xFFFFFFFF that is -1 gets chopped down to 0xFF, or 255 when viewed as an unsigned char. Use EXIT_SUCCESS and EXIT_FAILURE from stdlib.h, they will be portable.

    You should make result more than 40 chars long. The bare minimum would be for a-zA-Z, which is 52 characters total, plus one for the null, totaling 53. Just be safe and make it something like 64, then you can easily change your program to take all 3 ranges.

    Call strlen only once in check_validation. Use a variable to hold it's len and check later:
    Code:
    size_t len = strlen(pointer);
    if (len == 3)
    ...
    Your logic in check_validation is all kinds of screwy.

    Code:
    if ((pointer[i] - pointer[i-2]) > (('z' - 'a') || ('9' - '0')))
    That doesn't quite do what you think. It evaluates (('z' - 'a') first, finds it to be logically true, and skips checking '9' - '0' (due to short circuit evaluation). The logical value of 'z' - 'a' is 1, so you end up with if ((pointer[i] - pointer[i-2]) > 1), which you probably don't want. You would need to do:
    Code:
    if (pointer[i] - pointer[i-2] > 'z' - 'a' || pointer[i] - pointer[i-2] > '9' - '0')
    You could save yourself some work and make your code more readable by #include-ing <ctype.h> at the top of your program and using functions like islower, isupper and isdigit instead of pointer[i] >= 'a'.... Your logic is all messed up in there, and I think you're using break when you should be using continue.

    You're a little bit parentheses happy. Only use them when you need to alter the default order of operations, or when you really, really need them for clarity. Study operator precedence here: C Operator Precedence Table.

    I would probably split your check into 2 functions, one to check an individual range, and one for the whole user input. Something like:
    Code:
    int valid_range(char *range)
    {
        if range[1] is not a '-'  // range is missing a '-'
            return false
        if range[0] >= range[2]  // range specified backwards
            return false
        if range[0] and range[2] are lower case  // islower
            return true
        if range[0] and range[2] are upper case
            return true
        if range[0] and range[2] are digits
            return true
        return false  // default
    }
    
    int valid_input(char *input)
    {
        size_t len = strlen(input);
    
        if (len == 3)
            return valid_range(input);
        else if (len == 6)
            return valid_range(input) && valid_range(input+3);  // two ranges specified, check both
        // you can easily add code here to check 3 ranges if len == 9
        else
            return 0;  // false for failure
    }
    ...
    if (!valid input(argv[1])
        return EXIT_FAILURE;
    The last comment I will make is that you need better comments. Your comments should avoid stating the obvious or rephrasing the name of a function, and focus more on explaining the complicated pieces of code, and why your code does what it does. Pretend it will be read by somebody who is fluent in C, but has no idea what your program does. That's how you'll feel when you come back to it in 3 months.
    CommonTater likes this.

  7. #7
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,806
    Code:
    if (((strlen(pointer)) != 3) || (strlen(pointer) != 6))
        return -1;
    This code always results in a true outcome... the return will always happen.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  8. #8
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    Quote Originally Posted by hk_mp5kpdw View Post
    Code:
    if (((strlen(pointer)) != 3) || (strlen(pointer) != 6))
        return -1;
    This code always results in a true outcome... the return will always happen.
    Yeah i realized that and more mistakes so i decided to start again.
    Thanks to anduril462s feedback it works... kind of
    here's my code:
    Code:
    // my_expand.c
    /* program to take a given expression and parse
     * it to the entire range e.g. a-z = abcdef...xyz
     */
     
     #include <string.h>
     #include <ctype.h>
     #include <stdlib.h>
     #include <stdio.h>
     #include <math.h>
     
     #define TRUE 1                  // used for checking the input range
     #define FALSE 0
     
     void expand(char *s1, char *s2);
     int check_range(char *range);
     int check_input(char *range);              // two functions to check the input is valid for the program
     
     int main(int argc, char **argv)
     {
    	if (argc < 2 || argc > 2)
    	{
    		fprintf(stderr, "Usage: %s <expression> e.g. a-z, 0-9 or A-Z0-9\n", argv[0]);
    		return EXIT_FAILURE;   // if there are more than 2 args in input exit program
    	}
    	
    	char *input = argv[1];
    	char result[64];
    	
    	if (!check_input(argv[1]))
    	{
    		fprintf(stderr, "Usage: %s <expression> e.g. a-z, 0-9 or A-Z0-9\n", argv[0]);
    		return EXIT_FAILURE;
    	} else {
    		printf("The expression: %s\n", input);
    		expand(input, result);
    		printf("And the result: %s\n", result);
    		return EXIT_SUCCESS;
    	}
    }
    
    int check_range(char *range)          // check the range
    {
    	if (range[1] != '-')
    		return FALSE;                             // exit program if there is not a '-' where needed
    	if (range[0] >= range[2])
    		return FALSE;                             // exit program if range is backwards
    	if (islower(range[0]) && islower(range[2]))
    		return TRUE;                              // return true if range is lower case
    	if (isupper(range[0]) && isupper(range[2]))
    		return TRUE;                              // return true if range is upper case
    	if (isdigit(range[0]) && isdigit(range[2]))
    		return TRUE;                              // return true if range are digits
    	
    	return FALSE;             // default
    }
    
    int check_input(char *input)              // check the whole input
    {
    	int len = strlen(input);
    	if (len == 3)
    		return check_range(input);
    	else if (len == 6)
    		return check_range(input) && check_range(input+3);
    	else
    		return FALSE;
    }
    
    void expand(char *s1, char *s2)
    {
    	char value = s1[0];         // get the start value
    	int i, len, range;
    	len = strlen(s1);               // length of expression
    	range = s1[2] - s1[0];          // number of values to be copied
    	for (i = 0; i < range; i++)
    		s2[i] = value++;            // copy the values into s2
    	
    	if (len == 6)
    	{
    		i = range;                  // old range to carry on from where the above for loop left off
    		value = s1[3];              // new value (the first element of the second expression)
    		range = s1[5] - s1[3];      // number of values to be copied for the second expression
    		for (i ; i < range; i++)
    			s2[i] = value++;        // copy the range of the second expression into s2 (result)
    	}
    }
    When i type
    Code:
    my_expand a-z
    in CMD it gives the string abcde...xy, then after y there is some binary characters.
    And also it doesn't work when I request a range a-z0-9, the output is the same and i can't see where I'm going wrong, can anyone show me where I'm going wrong?
    Thanks

  9. #9
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,629
    range should be calculated as high - low + 1, so that's why there's no 'z' in your range. Also, you need to null terminate s2, since you're treating it as a string and not just an array of characters. Again, as a matter of good design, I would make a function that expands a single range, and a wrapper function that expands all ranges in your input, similar to the check functions.

  10. #10
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    Ok thanks
    So how would I terminate the s2 string? would it be
    Code:
    s2[sizeof(s2)-1] = 0;
    ??

    doesn't matter, I've done it.
    Last edited by cprog12; 06-14-2011 at 04:05 PM.

  11. #11
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,629
    Actually, your use of the sizeof operator could be problematic. The i variable is the index after the last character in your string. At the end of the expand function, just do:
    Code:
    s2[i] = '\0';

  12. #12
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    Yeah thanks it works but i'm still having problems with a-z0-9, It just doesn't recognize 0-9 and prints abcdef...xyz

  13. #13
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,629
    Post the new version of your expand function please. I'm guessing you put the null termination line before your if (len == 6) bit.

  14. #14
    Registered User
    Join Date
    Nov 2010
    Posts
    45
    Code:
    void expand(char *s1, char *s2)
    {
    	char value = s1[0];         // get the start value
    	int i, len, range;
    	len = strlen(s1);               // length of expression
    	range = s1[2] - s1[0] + 1;          // number of values to be copied
    	for (i = 0; i < range; i++)
    		s2[i] = value++;            // copy the values into s2
    	
    	if (len == 6)
    	{
    		i = range;                  // old range to carry on from where the above for loop left off
    		value = s1[3];              // new value (the first element of the second expression)
    		range = s1[5] - s1[3] + 1;      // number of values to be copied for the second expression
    		for (i ; i < range; i++)
    			s2[i] = value++;        // copy the range of the second expression into s2 (result)
    	}
    	s2[i] = '\0';                 // terminate the string with a null char
    }

  15. #15
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,629
    Quote Originally Posted by cprog12 View Post
    Code:
    void expand(char *s1, char *s2)
    {
    	char value = s1[0];         // get the start value
    	int i, len, range;
    	len = strlen(s1);               // length of expression
    	range = s1[2] - s1[0] + 1;          // number of values to be copied
    	for (i = 0; i < range; i++)
    		s2[i] = value++;            // copy the values into s2
    	
    	if (len == 6)
    	{
    		i = range;                  // old range to carry on from where the above for loop left off
    		value = s1[3];              // new value (the first element of the second expression)
    		range = s1[5] - s1[3] + 1;      // number of values to be copied for the second expression
    		for (i ; i < range; i++)
    			s2[i] = value++;        // copy the range of the second expression into s2 (result)
    	}
    	s2[i] = '\0';                 // terminate the string with a null char
    }
    The code in red can be removed, it's useless. You're having trouble because in your second loop, range is only 10, but i starts and 26. That causes the loop condition to fail so the loop never executes. You might need a second variable, say j, that loops from 0 to range, and set s2[i++] to value++.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 15
    Last Post: 06-09-2009, 03:19 AM
  2. Replies: 2
    Last Post: 02-26-2009, 11:48 PM
  3. Print function: sending a function.. through a function?
    By scarlet00014 in forum C Programming
    Replies: 3
    Last Post: 11-05-2008, 05:03 PM
  4. Replies: 14
    Last Post: 03-02-2008, 01:27 PM
  5. Replies: 9
    Last Post: 01-02-2007, 04:22 PM

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