VS Code versus cs50.dev

Question

I was write this code in Visual Studio and it's work okey. Everything is fine.
I was start to learn C in cs50.dev. Sometimes i write in cs50.dev. Because it's easy to take input from user. But a question give me a headache. Why same code give me an error. I was copy paste it.
I was write this code:

#include <stdio.h>
#include <ctype.h>
#include <string.h>
#include <stdlib.h>
#include <stdbool.h>
s
int add(char *name, int pow, char class);
int delete(char *name);
int print();

typedef struct addhero
{
    char name[50];
    int pow;
    char class;
    struct addhero *next;
} addhero;
addhero *first = NULL;
addhero *last = NULL;

int main()
{
    char yn = ' ';
    while (1){
        char hn[50];
        int hp = 0;
        char hc = ' ';
        printf("Enter hero name: ");
        scanf(" %s", hn);
        printf("Enter hero power: ");
        scanf(" %d", &hp);
        printf("Enter hero class: ");
        scanf(" %c", &hc);
        add(hn, hp, hc);
        printf("Keep going to add? (Y/N): ");
        scanf(" %c", &yn);
        if (islower(yn)) yn = toupper(yn);
        if (yn == 'N')
            break;
    }
    print();
}

int add(char *name, int pow, char class)
{
    if (strcmp(first->name, "\0") == 0){
        addhero *new = (addhero *)malloc(sizeof(addhero));
        strcpy(new->name, name);
        new->pow = pow;
        new->class = class;
        new->next = NULL;

        first = last = new;
    }
    else{
        addhero *new = (addhero *)malloc(sizeof(addhero));
        strcpy(new->name, name);
        new->pow = pow;
        new->class = class;
        new->next = NULL;

        last->next = new;
        last = new;
    }
    return 0;
}

int delete(char *name)
{
    addhero *prev = NULL;
    addhero *index = first;
    if (first == NULL){
        printf("There is no hero now.");
        return 0;
    }
    // Baştaki eleman siindiyse güncelle.
    if (strcmp(first->name, name) == 0){
        addhero *temp = first;
        first = first->next;
        free(temp);
        return 0;
    }
    while (index != NULL && !(strcmp(index->name, name))){
        prev = index;
        index = index->next;
    }
    if (index == NULL){
        printf("There is no hero in this name.\n");
        return 0;
    }
    prev->next = index->next;
    // Sondaki eleman silindiyse güncelle.
    if (strcmp(last->name, name) == 0){
        last = prev;
    }
    free(index);
    return 0;
}

int print()
{
    printf("\n-> ╠══ Your Heroes ══╬\n");
    printf("-----------------------\n");
    addhero *index = first;
    while (index->next != NULL)
    {
        printf("-> Hero: %s ══ %d | %c\n", index->name, index->pow, index->class);
        index = index->next;
    }
    printf("-> Hero: %s ══ %d | %c\n", last->name, last->pow, last->class);
    printf("-----------------------\n");
    return 0;
}

It's look okey in VS but in cs50 givin error: "Segmentation fault (core dumped)" So why is that? Can someone explain this please?

I expect to in my cs50.dev worspaces work as work as in VS. So i want to learn why is this error in cs50.dev.

Answer 1

The main issue is with this statement:

if (strcmp(first->name, "\0") == 0){

The first time this executes, first is NULL and so you have undefined behaviour. This should just be:

if (first){

There are a few bugs in your delete function as well:

• When the first element is deleted, there is no check whether this deleted element was the last one, and last remains pointing to it, even after it is freed. Again this may lead to undefined behaviour.

• The if (strcmp(last->name, name) == 0){ condition is not correct and will cause problems if you have two elements with that name. Then this statement will wrongly conclude that the last element was deleted, while actually the first occurrence of that name is being deleted. This condition should be:

  if (!index->next) {
  

• The !(strcmp(index->name, name) condition for the while loop is wrong. It should be the opposite: when strcmp is 0 (false), the loop should exit, not continue.

• Not a problem, but it is a pity that the while loop performs the same string comparison as was already done in the preceding if condition. You could omit that if block and perform a check after the loop -- to see whether the match is with the first element.

And there is one issue with the print function:

• It assumes the list as at least one entry, and so it will have undefined behaviour when the list is empty. Make the condition of the loop just (index) and remove that printf after the loop.

I would also avoid the repetition of code you have in add: create a separate function create that performs the malloc and initialises the allocated element.

Also consider: Do I cast the result of malloc?

Here are your functions (and added create) with those adaptations:

addhero *create(char *name, int pow, char class) 
{
    addhero *new = malloc(sizeof(*new));
    strcpy(new->name, name);
    new->pow = pow;
    new->class = class;
    new->next = NULL;
    return new;
}

int add(char *name, int pow, char class)
{
    printf("add '%s' with power %d and class '%c'\n", name, pow, class);
    addhero *new = create(name, pow, class);
    if (!first) {
        first = new;
    }
    else {
        last->next = new;
    }
    last = new;
    return 0;
}

int delete(char *name)
{
    addhero *prev = NULL;
    addhero *index = first;
    if (first == NULL) {
        printf("There is no hero now.");
        return 0;
    }
    while (index && strcmp(index->name, name)){
        prev = index;
        index = index->next;
    }
    if (!index){
        printf("There is no hero in this name.\n");
        return 0;
    }
    if (prev) {
        prev->next = index->next;
    }
    else {
        first = first->next;
    }
    if (!index->next) {
        last = prev;
    }
    free(index);
    return 0;
}

int print()
{
    printf("\n-> ╠══ Your Heroes ══╬\n");
    printf("-----------------------\n");
    addhero *index = first;
    while (index)
    {
        printf("-> Hero: %s ══ %d | %c\n", index->name, index->pow, index->class);
        index = index->next;
    }
    printf("-----------------------\n");
    return 0;
}

You should also review your main function so that you don't read larger strings than 50 (including terminator) into your hn variable. See Limit user to type certain amount of characters