current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I have some code where the purpose is the decide if a char is between quotes in an expression. For example, awk '{print $1}' has a { between quotes.

The function that I want to test is

int IBQplain(int pos, char *str, int offset);

int IBQdouble(int pos, char *str, int offset);

int IBQsingle(int pos, char *str, int offset) {
    int escaped = 0;
    for (; str[offset]; ++offset) {
        if (!escaped) {
            switch (str[offset]) {
                case '\\':
                    escaped = 1;
                case '\'':
                    return IBQplain(pos, str, offset + 1);
            }
        } else {
            escaped = 0;
        }
        if (pos == offset) {
            return 1;
        }
    }
    return 0;
}

int IBQdouble(int pos, char *str, int offset) {
    char ch;
    if (pos == offset)
        return 0;  /* Not within quotes */
    int escaped = 0;
    for (ch = str[offset]; ch; ch = str[++offset]) {
        if (!escaped) {
            switch (str[offset]) {
                case '\'':
                    return IBQsingle(pos, str, offset + 1);
                case '"':
                    return IBQdouble(pos, str, offset + 1);
                case '\\':
                    escaped = 1;
            }
        } else {
            escaped = 0;
        }
        if (pos == offset)
            return escaped;  /* Not within quotes, but may be single-escaped */
    }
    return 0; /* FIXME */
}

int IBQplain(int pos, char *str, int offset) {
    char ch;
    if (pos == offset)
        return 0;  /* Not within quotes */
    int escaped = 0;
    for (ch = str[offset]; ch; ch = str[++offset]) {
        if (!escaped) {
            switch (str[offset]) {
                case '\'':
                    return IBQsingle(pos, str, offset + 1);
                case '"':
                    return IBQdouble(pos, str, offset + 1);
                case '\\':
                    escaped = 1;
            }
        } else {
            escaped = 0;
        }
        if (pos == offset)
            return escaped;  /* Not within quotes, but may be single-escaped */
    }
    return 0; /* FIXME */
}


int isBetweenQuotes(int pos, char *str) {
    return IBQplain(pos, str, 0);
}

Write the test

#include "openshell.h"
#include "errors.h"
#test myQtest
fail_unless(isBetweenQuotes(11, "abc'asdqerfdsdxcvc'xc") == 1, "isBetweenQuotes function borked");
lm */

Compile it and run it

gcc -Wall -o bquotes-test util.c errors.c bquotes-test.c -lcheck -lsubunit -lrt -pthread -lm^C
dac@dac-Latitude-E7450:~/osh$ ./bquotes-test 
Running suite(s): Core
100%: Checks: 1, Failures: 0, Errors: 0

It seems ok. I make more tests

#include "openshell.h"
#include "errors.h"
#test myQtest
fail_unless(isBetweenQuotes(11, "abc'asdqerfdsdxcvc'xc") == 1, "isBetweenQuotes function borked");
fail_unless(isBetweenQuotes(5, "abcasdqerfdsdxcvcxc") == 0, "isBetweenQuotes function borked");
fail_unless(isBetweenQuotes(11, "ab'c'as'dqerfdsdxcvc'xc") == 1, "isBetweenQuotes function borked");

Compile and run it

checkmk tst_bquotes.check > bquotes-test.c
dac@dac-Latitude-E7450:~/osh$ gcc -Wall -o bquotes-test util.c errors.c bquotes-test.c -lcheck -lsubunit -lrt -pthread -lm
dac@dac-Latitude-E7450:~/osh$ ./bquotes-test 
Running suite(s): Core
100%: Checks: 1, Failures: 0, Errors: 0
share|improve this question
    
What's the purpose of your code? What problem does it solve? – Mast Jun 2 at 17:54
    
@Mast I added the purpose of the code: I have some code where the purpose is the decide if a char is between quotes in an expression. For example, awk '{print $1}' has a { between quotes. – Programmer 400 Jun 2 at 18:21
    
Ah, that explains a lot. – Mast Jun 2 at 18:23
    
@Mast Yes it solves a rather important problem for me...I must check if certain char are between quotes to be able to parse commands properly. Now I'm learning to write my first unit tests with check. – Programmer 400 Jun 2 at 18:25
up vote 2 down vote accepted

As OP did not explicitly state the review goals, here are some general ideas.

  1. Use const when referenced data is not changed. Allows for optimizations and defined behavior when the array address passed is const.

    // int IBQplain(int pos, char *str, int offset);
// int IBQdouble(int pos, char *str, int offset);
// int IBQsingle(int pos, char *str, int offset);
int IBQplain(int pos, const char *str, int offset);
int IBQdouble(int pos, const char *str, int offset);
int IBQsingle(int pos, const char *str, int offset);

  2. Use size_t as the type for array indexes. int maybe insufficient.

    // int IBQplain(int pos, char *str, int offset);
int IBQplain(size_t pos, char *str, size_t offset);

  3. As the functions only return 1 of 2 values, consider _Bool or bool

    #include <stdbool.h>
// int IBQplain(size_t pos, char *str, size_t offset);
bool IBQplain(size_t pos, char *str, size_t offset);

  4. As these functions appear to be stand-alone, suggest more pessimistic range checking:

    // if (pos == offset)
if (pos <= offset)
  return 0;  /* Not within quotes */

  5. ... otherwise if the below 3 are only helper functions to isBetweenQuotes(), they should be static.

    static int IBQplain(int pos, const char *str, int offset);
static int IBQdouble(int pos, const char *str, int offset);
static int IBQsingle(int pos, const char *str, int offset);

  6. Unclear why code accesses the array again instead of ch

    for (ch = str[offset]; ch; ch = str[++offset]) {
    if (!escaped) {
      // switch (str[offset]) {
      switch (ch) {

  7. Expect more test cases that have incomplete '' and "" pair combinations, especially onces that begin or end the string.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.