Which would you prefer to have to maintain?

Question

[To any mod considering deleting this question - please don't. Either my or Steve Jessop's code may well be helpful to reference from questions regarding reading text files using C.]

This question has generated the usual amount of sound and fury re the relative merits of C and C++. But something that never seems to get introduced into these discussions is actual code. I thought I would change that, so I set myself a small test:

Create a command line utility that reads zero or more files into memory and then outputs them in a random order. Each line must be output only once. The program to be written in C++98 or C89, using only their standard library functions.

The result is the two programs I present as answers below. I'd be grateful if you would vote on the one you would rather be tasked with maintaining.

Some notes:

• The code is written in my own idiomatic style. The only difference being that there are no comments and less vertical whitespace than I would normally use.

• The random number generator is naive, but is the same in both cases. so it dodn't seem worth improving at this point.

• The C program reads only lines up to a fixed length - I couldn't be bothered implementting a true variable length file reading function. It would probably have added about 10 more lines to the code.

• There is no check for memory exhaustion in either version.

---

Ah well, I thought it would get closed fairly quickly. Still, I enjoyed writing the code and seeng other people's code too - it seems to me that SO is lacking in that regard. Thanks to all who took part.

Answer 1

This is the C version - vote if you would like to maintain this:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#define START_LINES     100
#define MAX_LINE_LEN    1000

typedef struct {
    unsigned int mUsed, mSize;
    char ** mLines;
}  LineList;

LineList * MakeLineList( unsigned int nlines ) {
    LineList * ll = malloc( sizeof( LineList ) );
    ll->mUsed = 0;
    ll->mSize = nlines;
    ll->mLines = malloc( sizeof( char* ) * nlines );
    return ll;
}

void FreeLineList( LineList * ll ) {
    unsigned int i;
    for ( i = 0; i < ll->mUsed; i++ ) {
        free( ll->mLines[i] );
    }
    free( ll->mLines );
    free( ll );
}

void GrowLineList( LineList ** ll ) {
    LineList * newll = MakeLineList( (*ll)->mSize * 2 );
    unsigned int i;
    for ( i = 0; i < (*ll)->mUsed; i++ ) {
        newll->mLines[ newll->mUsed++ ] = (*ll)->mLines[ i ];
        (*ll)->mLines[ i ] = NULL;
    }
    FreeLineList( * ll );
    * ll = newll;
}

void AddLine( LineList ** ll, char * line  ) {
    if ( (*ll)->mUsed >= (*ll)->mSize ) {
        GrowLineList( ll );
    }
    (*ll)->mLines[ (*ll)->mUsed++ ] = line;
}

char * ReadLine( FILE * f ) {
    char tmp[ MAX_LINE_LEN ];
    if ( fgets( tmp, MAX_LINE_LEN, f ) == NULL ) {
        return NULL;
    }
    else {
        char * line = malloc( strlen( tmp ) + 1 );
        strcpy( line, tmp );
        return line;
    }
}

void AddFileContents( LineList ** ll, char * filename ) {
    FILE * f = fopen( filename, "r" );
    char * line;
    if ( f == NULL ) {
        fprintf( stderr, "file open failed: %s\n", filename );
        exit( 1 );
    }
    while( (line = ReadLine( f )) != NULL ) {
        AddLine( ll, line );
    }
    fclose( f );
}


int GetRand( int n ) {
    return rand() % n;
}

void Swap( char ** a, char ** b ) {
    char * tmp = * a;
    * a = * b;
    * b = tmp;
}

void Deal( LineList * ll ) {
    unsigned int i;
    for (i = 0; i < ll->mUsed; i++ ) {
        unsigned int r = GetRand( ll->mUsed - i );
        fprintf( stdout, "%s", ll->mLines[r] );
        Swap( &(ll->mLines[r]), &(ll->mLines[ (ll->mUsed - i) - 1 ]) );
    }
}

int main( int argc, char *argv[] ) {
    int i;
    LineList * ll = MakeLineList( START_LINES );
    for ( i = 1; i < argc; i++ ) {
        AddFileContents( &ll, argv[i] );
    }
    srand( time(0) );
    Deal( ll );
    FreeLineList( ll );
    return 0;
}

Answer 2

This is the C++ version - vote if you would like to maintain this:

#include <iostream>
#include <fstream>
#include <vector>
#include <string>
#include <ctime>
#include <cstdlib>
using namespace std;

typedef vector<string> LineList;

void AddFileContents( LineList & lines, const string & filename ) {
    ifstream ifs( filename.c_str() );
    if ( ! ifs.is_open() ) {
        cerr << "Cannot open file: " << filename << endl;
        exit(1);
    }
    string line;
    while( getline( ifs, line ) ) {
        lines.push_back( line );
    }
}

int GetRand( int n ) {
    return rand() % n;
}

void Deal( LineList & lines ) {
    for ( unsigned int i = 0; i < lines.size(); i++ ) {
        unsigned int r = GetRand( lines.size() - i );
        cout << lines[r] << "\n";
        swap( lines[r], lines[ (lines.size() - i) - 1 ]);
    }
}

int main( int argc, char *argv[] ) {
    LineList lines;
    for ( int i = 1; i < argc; i++ ) {
        AddFileContents( lines, argv[i] );
    }
    srand( time(0) );
    Deal( lines );
}

Answer 3

Here's what I'd prefer (one might add error checking for file opening, but I don't think this is strictly necessary, if you don't care):

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
#include <algorithm>
#include <iterator>
#include <ctime>
#include <cstdlib>

int main(int argc, char* argv[])
{
    std::srand(std::time(0));
    std::vector<std::string> lines;
    for (int i = 1; i != argc; ++i) {
        std::ifstream fin(argv[i]);
        std::string line;
        while (std::getline(fin, line)) {
            lines.push_back(line);
        }
    }
    std::random_shuffle(lines.begin(), lines.end());
    std::copy(lines.begin(), lines.end(), std::ostream_iterator<std::string>(std::cout, "\n"));
}

Answer 4

One thing I struggle a lot with is trying to figure out how much of the code we consider difficult is actually difficult, or is simply different than what we are used to. At this writing, there are 19,954 questions tagged c++, and 8,432 questions tagged c, indicating that C++ has gotten a lot more activity than C, so I expect a lot more familiarity with it, and a lot of the positive responses (I fully expect the C++ version will "win") will be due more to that familiarity than to better actual maintainability.

My preference is for the C version, which I'd justify by saying I prefer the implementations of the libraries to be more obvious, so that if bugs do show up I'll be able to very quickly drill down to where the bug is. But, really, it's probably because I work with a lot more C than C++.

Answer 5

These language comparisons based on specific code for a given problem are totally useless. For any specific problem there are a gazillion possible solutions in one language alone -- beginning with the choice of algorithm and not ending with code and library layout, variable names, etc. There are all kind of solutions, from abysmal ugly and inefficient in space and time to godly excellent and lightning fast.

What will the result of this survey tell you? Are you sure if you're comparing a good C solution with a bad C++ solution, the other way around, or something average? Have you even compiled and tested both solutions, are they working as expected? Do they at least implement the exact same algorithm?

Answer 6

I would by far prefer to maintain the C++ version. But then again, I am a C++ programmer. And so this question is missing an important point.

The C version is verbose and very explicit about everything that happens. That means there's more code to maintain, and more work for those of us who can read both versions.

But what if you know neither language to begin with? It doesn't take many minutes of tutoring before you can understand the C code because it is so verbose and simple and explicit.

The C++ version can potentially become a lot shorter and more concise, making it easier to maintain for a C++ programmer, but also filling it with gotchas and things that aren't immediately obvious if you're not a C++ programmer.

So for some purposes, the C version is more maintainable. That's why I argued against you in the other question. There's no doubt that you (Neil) and I, and all the usual answerers of C++ questions here on SO would by far prefer to maintain the C++ versions. But people who are new to C++ wouldn't necessarily agree. Short and concise code is nice if you already understand the language. But it also presents fewer hooks for understanding what's actually happening under the hood. The C version is much simpler, in that everything that happens is clearly spelled out when it happens. After you allocate memory with malloc, you initialize it, one field at a time, right there, for all to see. In C++, you just new something, and hey, somehow the right constructor (because of course, there's often more than one in a class) magically takes care of it.

Answer 7

To answer in a more general way: I'd prefer maintaining clean C++ code over clean C code, but if the C++ code was written by some "design patterns lover", I'll take the C code any day. Well, unless it abuses macros.

Answer 8

Alternative C version: I think Neil's LineList is unnecessarily complex. There's still no checking for out of memory, which is something I'd never leave in practice, but I handle arbitrary length lines. I do prefer UncleBens' C++ to this, and the difference is almost entirely down to C++'s superior standard library. In general, C++ has language-based advantages handling resources, but in this case freeing what was allocated isn't really a big deal.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#define START_LINES     100

typedef struct {
    unsigned int mUsed, mSize;
    char ** mLines;
} StringList;

void initList(StringList *list, unsigned int nlines ) {
    list->mLines = malloc( sizeof( char* ) * nlines );
    list->mUsed = 0;
    list->mSize = nlines;
}

void freeList(StringList *list) {
    free(list->mLines);
}

void freeContents(StringList *list) {
    unsigned int i;
    for (i = 0; i < list->mUsed; ++i) {
        free(list->mLines[i]);
    }
}

void growList(StringList *list) {
    list->mSize *= 2;
    /* not checking for failure sure makes realloc easy to use... */
    list->mList = realloc(list->mlist, sizeof(char *) * list->mSize);
}

void addString(StringList *list, char *line) {
    if ( list->mUsed >= list->mSize ) {
        growList(list);
    }
    list->mLines[list->mUsed++] = line;
}

char *readFile(FILE *f, StringList *lines) {
    long size;
    char *result, *tmp;
    int redd;

    fseek(f, 0, SEEK_END);
    size = ftell(f);
    fseek(f, 0, SEEK_SET);

    result = malloc(size+1);
    tmp = result;
    while (size > 0) {
        redd = fread(tmp, 1, size, f);
        /* error-checking required */
        tmp += redd;
        size -= redd;
    }
    {
        long i;
        size = tmp - result;
        tmp = result;
        for (i = 0; i < size; ++i) {
            if (result[i] == `\n`) {
                result[i] = 0;
                addLine(lines, tmp);
                tmp = result + i + 1;
            } else if (result[i] == `\r`) {
                result[i] = 0;
            }
        }
        /* nasty, without testing there's a high chance this is wrong */
        if (tmp != result + size) {
            result[size] = 0;
            addLine(lines, tmp);
        }
     }
     return result;
}

void AddFileContents( StringList *linelist, StringList *filelist, char * filename ) {
    FILE * f = fopen( filename, "r" );
    char * line, *file;
    if ( f == NULL ) {
        fprintf( stderr, "file open failed: %s\n", filename );
        exit( 1 );
    }
    addLine(filelist, readFile(f, linelist))
    fclose( f );
}

int GetRand( int n ) {
    return rand() % n;
}

void Swap( char ** a, char ** b ) {
    char * tmp = * a;
    * a = * b;
    * b = tmp;
}

void Deal( StringList * ll ) {
    unsigned int i;
    for (i = 0; i < ll->mUsed; i++ ) {
        unsigned int r = GetRand( ll->mUsed - i );
        fprintf( stdout, "%s\n", ll->mLines[r] );
        Swap( &(ll->mLines[r]), &(ll->mLines[ (ll->mUsed - i) - 1 ]) );
    }
}

int main( int argc, char *argv[] ) {
    int i;
    StringList lines;
    StringList files;

    initList(&lines, START_LINES);
    initList(&files, argc);

    for ( i = 1; i < argc; i++ ) {
        AddFileContents( &lines, &files, argv[i] );
    }
    srand( time(0) );
    Deal( &lines );

    freeContents(&files);
    freeList(&files);
    freeList( &lines );
    return 0;
}