function help

[cprog12]

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:

// 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:

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.

[CommonTater]

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.

[GReaper]

Assuming that you want "a-z" to expand to "abcdefghijklmnopqrstuvwxyz"

int i, j;
...
for (i = 'a', j = 0; i <= 'z'; i++, j++){
    s2[j] = (char)i;
}

s2[j] = '\0';

return s2;

[cprog12]

Ok, thanks for the help.

[cprog12]

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.

[anduril462]

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:

size_t len = strlen(pointer);
if (len == 3)
...

Your logic in check_validation is all kinds of screwy.

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:

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:

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.

[hk_mp5kpdw]

if (((strlen(pointer)) != 3) || (strlen(pointer) != 6))
    return -1;

This code always results in a true outcome... the return will always happen.

[cprog12]

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:

// 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

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

[anduril462]

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.

[cprog12]

Ok thanks
So how would I terminate the s2 string? would it be

s2[sizeof(s2)-1] = 0;

??

doesn't matter, I've done it.

[anduril462]

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:

s2[i] = '\0';

[cprog12]

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

[anduril462]

Post the new version of your expand function please. I'm guessing you put the null termination line before your if (len == 6) bit.

[cprog12]

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
}

[anduril462]

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++.