Thread: I'm sure I can solve this with a function I haven't heard about yet.

  1. #1
    Premier Member
    Join Date
    May 2010
    Location
    Antarctica
    Posts
    31

    I'm sure I can solve this with a function I haven't heard about yet.

    Hi,

    So, I have a file parser that, at one point, takes input of the form of "h(124) c(566) f(178)" and reads it into variable array current [7]. It will only accept l(), h(), a(), v(), c(), or f(), and stores each in a corresponding place in current. Now, I managed to make this work, but I think it could be condensed to a little less than one sixth of the code. Paraphrased:

    Code:
    int readstuffintovariables() {
        IGNOREWHITESPACE(props)
        if (buf == '&') {
            IGNOREWHITESPACE(props)
            if (buf == 'l') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [0] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            if (buf == 'h') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [1] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            if (buf == 'a') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [2] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            if (buf == 'v') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [3] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            if (buf == 'c') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [4] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            if (buf == 'f') {
                while (buf != '(')
                    buf = fgetc(props);
                for (int i = 0; i <= 2; i++) {
                    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
                }
                current [5] = atoi(num);
                while (buf == ')') {
                    buf = fgetc(props);
                }
                strcpy(num, "!!!");
                IGNOREWHITESPACE(props)
            }
            else
                return 2;
        }
        else
            return 1;
        return 0;
    }
    Yeah. Literally the only thing that changes is the index of current that it writes it to. Is there any way to solve this so it's tinier, with the exact same result? Maybe putting it in a for cycle would be the first step?
    Last edited by mszegedy; 05-22-2012 at 08:48 AM.

  2. #2
    Registered User claudiu's Avatar
    Join Date
    Feb 2010
    Location
    London, United Kingdom
    Posts
    2,094
    Why don't you write a function that contains the repetitive code and takes as argument the different index and pointers to whatever other variables are required for I/O?

    Then you can just call it as my_func(index,.....);. Also change your ifs to a switch, remember that characters are essentially represented as integers.
    1. Get rid of gets(). Never ever ever use it again. Replace it with fgets() and use that instead.
    2. Get rid of void main and replace it with int main(void) and return 0 at the end of the function.
    3. Get rid of conio.h and other antiquated DOS crap headers.
    4. Don't cast the return value of malloc, even if you always always always make sure that stdlib.h is included.

  3. #3
    Registered User
    Join Date
    Mar 2011
    Posts
    546
    Code:
    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
    since you are using fgetc, if a user types '1', you get the character '1', not the numeric value 1. so I think you would want to check against '1','2','3' etc. But there is an easier way to do that. try
    Code:
    while(!isdigit(num[1])) {
       num[i] = fgetc(props);
    }
    you have to #include <ctype.h> to get isdigit

  4. #4
    Premier Member
    Join Date
    May 2010
    Location
    Antarctica
    Posts
    31
    Quote Originally Posted by claudiu View Post
    Why don't you write a function that contains the repetitive code and takes as argument the different index and pointers to whatever other variables are required for I/O?

    Then you can just call it as my_func(index,.....);. Also change your ifs to a switch, remember that characters are essentially represented as integers.
    I don't want to make my variables global, though, and I'm not very good with pointers to arrays. I guess I could make it return the value to put into current, though! Thanks, I'll try that.

    Quote Originally Posted by dmh2000 View Post
    Code:
    while (num [i] != 0 || num [i] != 1 || num [i] != 2 || num [i] != 3 || num [i] != 4 || num [i] != 5 || num [i] != 6 || num [i] != 7 || num [i] != 8 || num [i] != 9)
                        num [i] = fgetc(props);
    since you are using fgetc, if a user types '1', you get the character '1', not the numeric value 1. so I think you would want to check against '1','2','3' etc. But there is an easier way to do that. try
    Code:
    while(!isdigit(num[1])) {
       num[i] = fgetc(props);
    }
    you have to #include <ctype.h> to get isdigit
    Hmm, you're right about that. I meant to put those in single quotes. 2 AM is definitely not the most productive time to code. Thanks for telling me about isdigit, though; much cleaner!

  5. #5
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    A dislike for global variables is good, but it looks like props, buf, num and current appear to be globals anyway. Also, avoiding "difficult" stuff like pointers to arrays (which you don't even really need) is not the way to improve your skills. I say you don't need them because, if you want to change the values in an array, you only need to pass a pointer to it's first element. Think of the strcpy function, it doesn't take a complicated char (*dest)[] parameter, just a char *dest.

    This basically looks like you are scanning a file for specifically formatted data. How about using fscanf (note, this is untested, it's purely conceptual):
    Code:
    char type[2];  // one char plus a null terminator
    int value, index;
    if (fscanf(props, "%1[lhavcf](%d)", type, &value) == 2) {  // make sure fscanf worked
        switch (type[0]) {
            case 'l':
                index = 0;
                break;
            ...
        }
        current[index] = value;
    }
    The %1[lhavcf] grabs a string that is 1 character long, and consists only of the characters inside the [ ], namely 'l', 'h', 'a', 'v', 'c', 'f'. That character/string must be followed by an open parenthesis, an integer and a close parenthesis. You can limit the number of characters in the integer in a similar manner to limiting the string to one character.

    Alternatively, you could make a string of valid chars for type, and use strchr with some pointer arithmetic to calculate the index.

  6. #6
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    Your while loop on line 9, and all the other ones like it are infinite loops.
    This much of it:
    Code:
    num [i] != 0 || num [i] != 1
    amounts to true because there do not exist any values for which it is false.
    Zero? Nope zero is not equal to 1, so we get false | true ==> true.
    One? Nope one is not equal to zero, so we het true | false ==> true.
    Two? Nope...
    Have a think about it and see if you can work out the mistake.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  7. #7
    Premier Member
    Join Date
    May 2010
    Location
    Antarctica
    Posts
    31
    Quote Originally Posted by iMalc View Post
    Your while loop on line 9, and all the other ones like it are infinite loops.
    This much of it:
    Code:
    num [i] != 0 || num [i] != 1
    amounts to true because there do not exist any values for which it is false.
    Zero? Nope zero is not equal to 1, so we get false | true ==> true.
    One? Nope one is not equal to zero, so we het true | false ==> true.
    Two? Nope...
    Have a think about it and see if you can work out the mistake.
    Yeah, sorry, I meant to && them. :P I'm so absentminded sometimes... it's all a moot point, though, because I'm using isdigit(num [i]) now.
    Last edited by mszegedy; 05-23-2012 at 02:56 PM.

  8. #8
    Premier Member
    Join Date
    May 2010
    Location
    Antarctica
    Posts
    31
    Quote Originally Posted by anduril462 View Post
    A dislike for global variables is good, but it looks like props, buf, num and current appear to be globals anyway. Also, avoiding "difficult" stuff like pointers to arrays (which you don't even really need) is not the way to improve your skills. I say you don't need them because, if you want to change the values in an array, you only need to pass a pointer to it's first element. Think of the strcpy function, it doesn't take a complicated char (*dest)[] parameter, just a char *dest.

    This basically looks like you are scanning a file for specifically formatted data. How about using fscanf (note, this is untested, it's purely conceptual):
    Code:
    char type[2];  // one char plus a null terminator
    int value, index;
    if (fscanf(props, "%1[lhavcf](%d)", type, &value) == 2) {  // make sure fscanf worked
        switch (type[0]) {
            case 'l':
                index = 0;
                break;
            ...
        }
        current[index] = value;
    }
    The %1[lhavcf] grabs a string that is 1 character long, and consists only of the characters inside the [ ], namely 'l', 'h', 'a', 'v', 'c', 'f'. That character/string must be followed by an open parenthesis, an integer and a close parenthesis. You can limit the number of characters in the integer in a similar manner to limiting the string to one character.

    Alternatively, you could make a string of valid chars for type, and use strchr with some pointer arithmetic to calculate the index.
    Wooow. That's exactly the sort of thing that I'm looking for! Thanks, it's always good to learn new things. Jesus, this is pretty spot-on. That's brilliant. Thanks a lot! Oh, and it seems you know something about passing arrays into subroutines. So, I write the name of the array, and declare it with a single dereference as an array in the argument when I'm declaring the subroutine?

  9. #9
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    Quote Originally Posted by mszegedy View Post
    So, I write the name of the array, and declare it with a single dereference as an array in the argument when I'm declaring the subroutine?
    If I understand you correctly, yes, that is how you do it. Post an example if you want, I will tell you if you're correct.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 4
    Last Post: 10-11-2010, 10:16 AM
  2. Replies: 4
    Last Post: 06-04-2007, 05:37 PM
  3. Why the hell haven't I used WinAmp before?
    By cboard_member in forum A Brief History of Cprogramming.com
    Replies: 20
    Last Post: 08-22-2006, 02:25 AM
  4. I haven't seen before! getch() doesn't work!
    By StefanA. in forum C++ Programming
    Replies: 8
    Last Post: 02-07-2003, 05:43 AM