3
\$\begingroup\$

As some of you might know I'm working my way through The C Programming Language as recommended by the "The Definitive C Book Guide and List" on StackOverflow.

I just finished my version of squeeze(s1, s2), a function to remove all occurrences of s2 in s1 and was hoping to get some feedback on my solutions.

This exercise is part of the second chapter (ex 2.4) so we haven't really gone through pointers and de-referencing yet. So basically all experience I have with the language is with using basic and primitive data types and the previous feedback I've received here!

All sort of feedback would be greatly appreciated; syntactical changes, algorithmic shortcomings etc. Just keep in mind that I just recently started out with the language!


Here is the code:

/*
    Exercise 2.4
    squeeze (s1, s2): Remove all characters of s2 in s1.
    INPUT : s1.length >= s2 > 0.
    OUTPUT: The rest of s1 after deleting all occurances of letters in s2.
*/

#include <stdio.h>

void squeeze (char s1[], const char *s2);  /* Returns (by-ref) the resulting string s1 after removing all occurences of s2. */
int toUpper(char c);                       /* Returns the numerical representation of a hexadecimal digit. */
int contains(char toCheck, const char *toRemove); /* Function to see if toRemove contains toCheck. */


int main () {
    char s1[] = "I am a test.\0";
    const char *s2 = "AE\0";
    printf("Before manipulation: %s\n", s1);
    squeeze(s1, s2);
    printf("After manipulation:  %s", s1);
}

/*
    Returns the (by-ref) resulting string s1 after removing all occurences
    of letters in s2.
*/

void squeeze (char s1[], const char *s2) {
    int index, left_shift;

    for (index = 0, left_shift = 0; s1[index] != '\0'; index++) {
        if (contains(s1[index], s2))
            continue;
        s1[left_shift] = s1[index];
        left_shift++;
    }
    s1[left_shift] = '\0';
}
/*
    Returns the upper-case representation of char c.
*/

int toUpper (char c) {
    if (c >= 'a' && c <= 'z')
        return c - 32;
    else
        return c;
}

/*
    Returns an indicator (integer) of if
    the string toRemove contains toCheck.
*/
int contains(char toCheck, const char *toRemove) {
    int index = 0, c = 0;

    while (toRemove[index] != '\0') {
        if ((c = toUpper(toCheck)) == toRemove[index]){
            return 1;
        }
        index++;
    }
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Take a look at strpbrk() and strspn() which are useful for this task. \$\endgroup\$ – William Morris May 3 '17 at 20:55
  • \$\begingroup\$ Also, I don't see why there is any need to change case. The spec seems to say nothing about case-insensitivity. \$\endgroup\$ – William Morris May 3 '17 at 21:06
3
\$\begingroup\$

The code is relatively neat and appears to work properly, so you seem to be well on your way to mastering C. Here are some things that may help you on that road.

Strings are automatically terminated

The code currently includes these two lines:

char s1[] = "I am a test.\0";
const char *s2 = "AE\0";

However, the explicit '\0' at the end of these strings is not necessary. A C string is automatically terminated with an implicit '\0' character, so these lines would normally be written as:

char s1[] = "I am a test.";
const char *s2 = "AE";

Move function bodies to avoid declaration

Modern C no longer requires function declarations before every function definition. As long as, reading from top to bottom, each function definition does not refer to anything not yet defined, the function declarations may be omitted. So in this program, if you put the function definitions in the order toUpper, contains, squeeze and main, you can omit the declarations. This is handy because it means you only need to maintain the function signature in one place rather than two.

Use bool instead of int where appropriate

The contains function returns 1 if the value is found or 0 otherwise. That is, it's essentially returning a boolean value. You can make that more clear by using #include <stdbool.h> and writing the function as:

bool contains(char toCheck, const char *toRemove) { /* ... */ }

Then the return statements can be return true; and return false; As documented here, bool has been standard C since 1999.

Use standard functions

I know this is a learning exercise and you're just starting, but it's generally encouraged to use standard functions rather than duplicating them. For instance, toupper has been part of the standard since 1989.

Avoid creating unneeded variables

The variable c in the contains() routine is not really needed. Instead of this:

if ((c = toUpper(toCheck)) == toRemove[index]){

You can write this:

if (toUpper(toCheck) == toRemove[index]){
\$\endgroup\$
  • \$\begingroup\$ Cheers! I knew about most of the things except the declarations. I would 100% be using the standard functions and datatypes (boolean values), but as you mentioned it was a training exercise. Does all versions of C come bundled with stdbool and ctype? I'm learning C to learn to read C-exploits from exploitdb more efficiently, so I'm curious to hear if I can expect those libraries to be included in all versions of C. \$\endgroup\$ – geostocker Apr 26 '17 at 12:38
  • \$\begingroup\$ I've updated my answer to address that, but essentially, yes. The C99 standard is now 18 years old so pretty much any current compiler will have all of these features. \$\endgroup\$ – Edward Apr 26 '17 at 12:42
  • \$\begingroup\$ That edit was fudging BRILLIANT!. I had no clue about the function calls and unneeded variables! The way I've done it has been how the book as suggested. :) \$\endgroup\$ – geostocker Apr 26 '17 at 12:45
  • \$\begingroup\$ Since you seem to know your way around C, how would you suggest a Python docstring equivalent in C should be written? Ie. how should I properly document the functionality of a function? \$\endgroup\$ – geostocker Apr 26 '17 at 12:49
  • 1
    \$\begingroup\$ I use (and recommend) Doxygen. The many examples shown there should give you a good idea about how to document functions, even if you don't actually use that tool. \$\endgroup\$ – Edward Apr 26 '17 at 12:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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