5
\$\begingroup\$

Looking at my code, I feel like it is not instantly clear for someone who did not write it. I feel like readability will be important for me in the future, if I'm ever to write code on an enterprise, or open source project. Do you have any advice as to how I can make this and future C more readable?

#include "datatypes.h"
#include <stddef.h>

//Changes the node's child (if it doesn't exist) to newChild
//or appends newChild to the end of the node's child's sibling list
void addChild ( treeNode *root, treeNode *newChild ) {
  treeNode *temp;
  if ( root->child == NULL ) {
    root->child = newChild;
  } else {
    temp = root->child;
    while ( temp->sibling != NULL ) {
      temp = temp->sibling;
    }
    temp->sibling = newChild;
  }
}

//Returns a new treeNode with default data values or {0,NULL,NULL}
treeNode newNode () {
  treeNode node = {0, NULL, NULL};
  return node;
}

datatypes.h

typedef struct treeNode {
  int n;
  struct treeNode *child;
  struct treeNode *sibling;
} treeNode;

treeNode newNode (); //Returns a treenode with default {0,NULL,NULL}
void addChild ( treeNode *t, treeNode *c ); //Adds child to the given  treeNode
\$\endgroup\$
  • 5
    \$\begingroup\$ Don't use such variable names (t, j) use descriptive names like tree, root, node ... and t->child is more readable than (*t).child \$\endgroup\$ – Alter Mann Oct 15 '15 at 18:43
  • \$\begingroup\$ Leading _ is a problem. May conflict with future keywords: see stackoverflow.com/questions/26217029/macro-naming-convention/… \$\endgroup\$ – chux - Reinstate Monica Oct 15 '15 at 19:18
  • 2
    \$\begingroup\$ I have rolled back two of your edits. Please see What to do when someone answers. \$\endgroup\$ – 200_success Oct 15 '15 at 19:58
  • \$\begingroup\$ BTW the comments are awful. They look like a mangled translation of your C code to English. Don't do that! Make your comments more high-level, e.g. your comment "Adds child to the given treeNode" is perfectly good. If someone wants to know how your code does that, he can run it in a debugger (or trace the code's execution in one's mind). Also, the comment on newNode is similarly long-winded, useless and error-prone - remove it or replace it with something like "Creates an empty tree". Also, the comment "and returns the root node" is incorrect (obsolete?) - remove it. \$\endgroup\$ – anatolyg Oct 15 '15 at 20:07
2
\$\begingroup\$

The code is pretty clear for anybody with C background. One suggestion though - there is no need for temp; you may safely reuse node:

    node = node->child;
    while (node->sibling) {
        node = node->sibling;
    }
    node->sibling = child;
|improve this answer|||||
\$\endgroup\$
4
\$\begingroup\$

Comment, Comment, Comment. If you want any other person on this blue planet to know whats going on in your code in the easiest way possible, comment your code.

// This is what the code does in the next line.
/* I'm going to write a block of text to tell you the purpose of what this function
is trying to accomplish */

Your code doesn't have to be pretty or legible for that matter, but a few comments go along way.

Also, whitespace for your if and while statements are appealing to the eye and allow the reader to break apart your conditional statements. Trying to cram as much as you can on one line causes headaches when trying to interpret them.

|improve this answer|||||
\$\endgroup\$
  • 4
    \$\begingroup\$ I disagree. Comments are often more a code smell than a helpfull hint. Also, they tend to get wrong over time. Try to refactor the code so that your intent becomes clear (good variable names, good function names, easy-to-read-coding style, short functions, etc.). \$\endgroup\$ – Peter Schneider Oct 15 '15 at 18:56
  • 4
    \$\begingroup\$ @PeterSchneider: Real coders do not comment, huh? Sorry, but this is... well... just wrong. Code only tells you "how". Comments should of course not duplicate that, they should tell you "what" (one level up). And then the documentation should tell you the "why". You can write the clearest code in the world, that does not make comments unnecessary. At the very least, they can provide an indication for the poor sod debugging your code where your "how" didn't actually do "what" you intended... \$\endgroup\$ – DevSolar Oct 15 '15 at 19:04
  • 1
    \$\begingroup\$ I've been working on two commercial products with a10 years plus history. Sometimes it's a funny game to trace a comment back in time if it made sense back then and when they become missleading. I know this isn't optimal but people tend to ignore documentation and dive directly into the code fixing the current problem and do not update the comment. Maybe I had bad luck, maybe its typical. Don't get me wrong good comments are great - but bad ones are worse than none. And I comment my code if it is necessary BTW ;) (math stuff, why this nasty workaround and not the obvious way, etc.) \$\endgroup\$ – Peter Schneider Oct 15 '15 at 20:55
  • 1
    \$\begingroup\$ @KillaBytes: "the product" is the final binary with all of it's new features and bugfixes compared to the last version. Comments are internal documentation and "the company" doesn't get money for it. Managers are pushing for code - not for comments ;) As a professional you can (and should!) try to improve the internal code quality. But management will almost never ask for this and it will only work in small steps and it is a long march. \$\endgroup\$ – Peter Schneider Oct 15 '15 at 21:18
  • 2
    \$\begingroup\$ @KillaBytes: officially, I only do what my manager is asking for. Inofficially I try to improve the code base with every commit (this rarely means comments for me but sometimes they are an improvement). The trick is to find a way between. You do not get paid for 8 hours maintenance a day. If you do zero, your code base is rotten in 2 years. \$\endgroup\$ – Peter Schneider Oct 15 '15 at 21:42
0
\$\begingroup\$

You can easily simplify your addChild-method:

void addChild ( treeNode *root, treeNode *newChild ) {
    treeNode** p = &root->child;
    while(*p)
        p = &p[0]->sibling;
    *p = newChild;
}

And there's no reason for newNode to exist as it's trivial (C99 being available for compound-literals):

treeNode newNode () {
    return (treeNode){0}; // C needs at least one initializer
}
|improve this answer|||||
\$\endgroup\$

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.