Return Styles: Pseud0ch, Terminal, Valhalla, NES, Geocities, Blue Moon. Entire thread

[CodeCritique] Criticize my Linked List

Name: Anonymous 2011-08-19 20:33

I have implemented linked lists in C as a learning exercise.  I'm a noob ass so tell me how my implementation sucks and I will fix it.  I didn't have a particular use-case in mind when I wrote it.

Header:
// llist.h
#ifndef LLIST_H_
#define LLIST_H_

struct LList;
typedef struct LList LList;

struct LLNode;
typedef struct LLNode LLNode;

struct LList {
    LLNode *front, *back;
};

struct LLNode {
    void *data;
    LLNode *next;
};

LList* llist_new(LList **self);
void llist_add(LList *self, void *v);
void llist_remove(LList *self, LLNode *node);
void llist_insert(LLNode *node, void *v);
void llist_destroy(LList *self);

#endif


C file:
// llist.c
#include <assert.h>
#include <stdlib.h>

#include "llist.h"

// Initialize a new, empty LList, in dynamically allocated memory.
LList *llist_new(LList **self) {
    *self = malloc(sizeof (LList));
    if (self == NULL) { return NULL; }

    (*self)->front = NULL;
    (*self)->back = NULL;

    return *self;
}


// Append a value to the LList.  Creates a new node.
void llist_add(LList *self, void *v) {
    LLNode *node = malloc(sizeof (LLNode));
    assert(node != NULL);

    node->data = v;
    node->next = NULL;

    // Put this node at the back of the list.
    if (self->front != NULL) {
        self->back->next = node;
        self->back = node;
    } else {
        self->back = node;
        self->front = node;
    }
}


// Iterate over the LList to locate a node, and then remove it from the list.
void llist_remove(LList *self, LLNode *node) {
    LLNode *iter_node = self->front;
    LLNode *prev_node = NULL;

    while (iter_node != NULL) {
        if (iter_node == node) {
            if (prev_node != NULL) { prev_node->next = iter_node->next; }
            else { self->front = NULL; }
            free(iter_node);
            return;
        }
        prev_node = iter_node;
        iter_node = iter_node->next;
    }

}


// Insert a value after a particular node.
void llist_insert(LLNode *node, void *v) {
    LLNode *next = node->next;
    LLNode *new_node = malloc(sizeof (LLNode));
   
    new_node->data = v;
    new_node->next = next;
   
    node->next = new_node;
}


void llist_destroy(LList *self) {
    LLNode *iter_node = self->front;

    while (iter_node != NULL) {
        LLNode *next = iter_node->next;
        free(iter_node);
        iter_node = next;
    }

    free(self);
}


Testcase:
// llist_test.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "llist.h"

#ifdef _WIN32
char *strdup(char *s);
#endif

int main(void) {
    LList *phone_nums;
    LLNode *iter;

    if (llist_new(&phone_nums) == NULL) { printf("Out of memory."); exit(1); }
    llist_add(phone_nums, strdup("123-4567")); // Yes, I should free() these later...
    llist_add(phone_nums, strdup("890-1234"));
    llist_add(phone_nums, strdup("567-8901"));
    llist_add(phone_nums, strdup("234-5678"));
       
        LLNode *temp = NULL;
    iter = phone_nums->front;
    while (iter) {
        printf("%s\n", (char*)iter->data);
        if (!strcmp("890-1234", (char*)iter->data)) { temp = iter; }
        iter = iter->next;
    }
    puts("");
   
    llist_insert(temp, strdup("BEEP BOOP"));
   
    iter = phone_nums->front;
    while (iter) {
        printf("%s\n", (char*)iter->data);
        if (!strcmp("567-8901", (char*)iter->data)) { temp = iter; }
        iter = iter->next;
    }
    puts("");
   
    llist_remove(phone_nums, temp);
   
    iter = phone_nums->front;
    while (iter) {
        printf("%s\n", (char*)iter->data);
        if (!strcmp("567-8901", (char*)iter->data)) { temp = iter; }
        iter = iter->next;
    }
   
   
    llist_destroy(phone_nums);
    return 0;
}

#ifdef _WIN32
char *strdup(char *s) {
        char *p = malloc(strlen(s) + 1);
        strcpy(p, s);
        return p;
}
#endif

Name: Anonymous 2011-08-19 20:38

CRITIQUE MY ANUS

Name: Anonymous 2011-08-19 20:42

if it aint Lisp, it's crap.

Name: Anonymous 2011-08-19 20:44

>>3

THIS

Name: Anonymous 2011-08-19 20:44

>>3

+1

Name: Anonymous 2011-08-19 20:46

>>3-5 same fucking lithpfaggot
fuck off and die you fucking shitpushing faggot

Name: Anonymous 2011-08-19 20:50

x == NULL -> !x
x != NULL -> x

I prefer to keep structs explicit, so I'd write struct llnode *x etc.

And NULL can be written as just 0.

llist_new() also has a weird invocation. I'd just write it as

struct LList *llist_new() {
 struct LList *l = malloc(sizeof(struct LList));
 if(l) {
  l->front = 0;
  l->back = 0;
 }
 return l;
}

Name: Anonymous 2011-08-19 21:12

you forgot the garbage collector.

Name: Anonymous 2011-08-19 21:17

>>8
free(iter_node);
!

Name: Anonymous 2011-08-19 21:24


(define cons (lambda (car cdr) (lambda (k) (k car cdr))))
(define car (lambda (c) (c (lambda (x _) x))))
(define cdr (lambda (c) (c (lambda (_ x) x))))
(define uncons (lambda (c) (c values)))

Was it that difficult?

Name: Anonymous 2011-08-19 21:26

>>10
That appears to be a language that is not C, sir.

Name: Anonymous 2011-08-19 21:33

>>7
And NULLcan be written as just 0
Don't you realize how monumentally retarded this is?

Name: Anonymous 2011-08-19 21:36

>>12
Nope.

Name: Anonymous 2011-08-19 21:42

>>12
NULL is 0 at least in the source code.
Read this:
http://www.lysator.liu.se/c/c-faq/c-1.html

Name: Anonymous 2011-08-19 21:42

>>11
The implementation of closures is left as an exercise for the reader.

>>12-13
(void*)0 is guaranteed to be NULL, but 0 is not.

Name: Anonymous 2011-08-19 21:44

>>13
Then you're a fucking moron, holy shit, please don't have children but if you do please put them up for adoption as it is probably against the Geneva convention to have someone as retarded as you raise children you're dumber than the African Americans who shoot each other over gang colors in South Central holy shit are you fucking kidding me how can you not realize something that simple why don't you write out every sizeof as well then holy shit in fact why don't you just expand every macro while programming let me guess you're the type who writes 1 instead of sizeof(char) just because it says it's always 1 in the standard well I got news for you mister you are a fucking dickhead please never write code or post again, actually please kill yourself we don't need more fast food burger flippers posting on this board go play with some fucking barbie dolls you fucking mental midget go suck a fat cock you fucking bitch seriously FUCK OFF FAGGOT.

Name: Anonymous 2011-08-19 21:45

>>6
The autism is really flaring up in this one today.

Name: Anonymous 2011-08-19 22:09

>>17
Don't you mean >>16?

Name: Anonymous 2011-08-19 22:13

>>16
you'd have a point if NULL was typed, but it's just a #define.

0x might be changing this. Not sure. Couldn't give 2 shits about C++ anymore.

Name: Anonymous 2011-08-19 22:15

>>16
I do consider char to be 8 bits because my code runs on sane platforms -- for everything else I use uintXX_t due to compiler differences.

I don't consider 0 == NULL to be a large problem since you can just not map a page at virtual address 0 so that dereferencing null will cause a segfault.

now go fuck yourself you fucking faggot

Name: Anonymous 2011-08-19 22:23

>>19
u mena C1X (this is not C++)

Name: Anonymous 2011-08-19 22:28

>>18
No, I really do mean >>6

Name: Anonymous 2011-08-19 23:50

>>20
That's why your code will not work 10 years from now. Fucking kill yourself.

Name: Anonymous 2011-08-20 1:37

>>20
You think it's cool to needlessly obfuscate your code with stuff most people know? If you're going to obfuscate at least do it right instead of some half-assed faggotry.

Name: Anonymous 2011-08-20 6:58

obfuscate
stuff most people know
Choose one.

Name: Anonymous 2011-08-20 10:08

Update

LList *llist_new(void) {
    LList *self = malloc(sizeof (LList));
    if (self == NULL) {
                return NULL;
        }

    self->front = NULL;
    self->back = NULL;

    return self;
}

Name: Anonymous 2011-08-20 10:18

Awesome code. Great size. Looks concise. Efficient. Elegant. Keep us all posted on your continued progress with any new code factoring or compiler optimization. Show us what you got man. Wanna see how freakin' expressive, efficient, concise and elegant you can get. Thanks for the motivation.

Name: Anonymous 2011-08-20 10:48

>>25
That's the point, he's only obfuscating it for people who hardly know any programming and it makes him look like a fucking retard to everyone else.

Name: Anonymous 2011-08-20 11:53

This is a queue. Not a list.

Name: lolwrong 2011-08-20 14:29

>>29
void llist_insert(LLNode *node, void *v);

Name: Anonymous 2011-08-20 16:11

>>26
You don't need two returns.

Name: Anonymous 2011-08-20 16:21

>>31
I want to be sure.

Name: Anonymous 2011-08-20 18:33

>>31
Ok.  Show me how you would do it.

Name: Anonymous 2011-08-20 21:07

>>33
Not >>31, but

LList *llist_new(void) {
    LList *self = malloc(sizeof (LList));
    if (self) {
        self->front = self->back = NULL;
    }
    return self;
}

Name: Anonymous 2011-08-21 2:06

>>34
Yes, but this version is less explicit about what can be returned. self is ambiguous in this implementation; you'd have to do the extra work of seeing and understanding that it was malloc'd and could fail, and what the consequences of that failure mean. The other version makes an explicit distinction -- either you're getting a(n) LList object, or you're getting NULL, just by glancing.

Name: Anonymous 2011-08-21 8:15

>>35
If self is 0 you automatically fall through and return it.

Otherwise you go through the rest of the init.

It's obvious to anyone who's done a sufficient amount of C programming and much simpler.

You are just a retard.

Name: Anonymous 2011-08-21 8:31

why do you need no zero out the members? just do:
LList *llist_new(void) {
    return malloc(sizeof (LList));
}


also, that's the common malloc idiom:
T *p = malloc(NMEMB * sizeof *p);

Name: Anonymous 2011-08-21 8:38

>>37
s/no/to/

Name: Anonymous 2011-08-21 8:46

>>31,35
Explicitly returning NULL is a superior way of checking the malloc return code. What if you malloc n things, are you going to nest n if tests? Holy shit you don't know even know basic error checking, you're a fucking retard, please get the fuck out and never come back, you post shit all the time, you give horrible advice and you're an absolute shit programmer.

You will never be a good programmer, just leave.

Name: Anonymous 2011-08-21 11:47

>>37
lol @ lousy C programmer

You need to set the members because the pointer you get from malloc is not guaranteed to point to a bunch of 0 bytes.

See: http://codepad.org/Iiod5BIM

Newer Posts
Don't change these.
Name: Email:
Entire Thread Thread List