[C] deleting node from graph

Applications Programming
1

Using adjacency lists, I implemented this operation as follows:

/* remove the first node it finds with value 'val' in L list */
edge *removeEdge (edge *L, int val)
{
      if( !L ) return NULL;

      if( L->key == val )
      {
          edge *temp = L->next;
          free (L);
          return temp;
      }

      L->next = removeEdge (L->next, val);

      return L;
}

/* deletes node with key u from G */
int g_deleteNode (graph *G, int u)
{
    int i;     
    
    if( G != NULL && (u >= 0 && u < G->nv) )
    {   
        for( i = 0; i < G->nv; i++ )
             /* because removeEdge is a list node removing function */
             G->adj->next = removeEdge (G->adj->next, u);
        
        for( i = u; i < G->nv; i++ )
             if( i + 1 < G->nv ) G->adj = G->adj[i+1];
        
        free(G->adj);
        
        G->nv--;
               
        return 1;
    }
    
    else
    {
        if( !G ) printf("Error: null graph.\n");
        else printf("Error: edge does not belong to graph.\n");        
        return 0;
    }
}

Fact is, sometimes it crashes: could you help me understand why?

And is this a good implementation, or could I do better?

Thanks in advance.

2

I need to see your data structures too.

3

Oh, you're right. :stuck_out_tongue:

/* edge */
typedef struct edge
{
        int key,            /* node v of edge uv */
        struct edge *next;  
} edge;

/* graph */
typedef struct graph
{
        int nv;      /* number of nodes */
        edge **adj;  /* array of adjacency lists */
} graph;

Very essential.

4

Hi Luke,

I didn't spend much time with your program, but it occured to me that these lines look rather suspicious:

 
for( i = u; i < G->nv; i++ )
    if( i + 1 < G->nv ) G->adj = G->adj[i+1];
free(G->adj);

Are you not supposed to free G->adj [u]first, and then perform the shifting G->adj[i-1] = G->adj [i]for i=u+1 to G->nv ?

Is your graph directed, or undirected? What are the operations on the graph you want to be most efficient?

Cheers, Lo�c

1 Like
5

Hi Loic, tanks for your reply.

So I suppose doing it the opposite way is not correct, but why?

My graph is undirected and this is the operation I'd like to be the most efficient.

6

Because:

  • you're freeing G->adj[i], not G->adj[u].
  • If you would free G->adj[u], the pointer wouldn't be correct since you have already shifted.

You could perhaps get your code working by saving G->adj [u]before the shift; and then deleting it.

As your graph is undirected, there is many possible improvements. I'll try to post a few ideas tomorrow. But for now, it's bed time :slight_smile:

Cheers, Lo�c

---------- Post updated 06-19-11 at 06:15 AM ---------- Previous update was 06-18-11 at 10:31 PM ----------

Hi Luke,

here a few idea to improve the algorithm performance, given your data structure and the fact that the graph is undirected:

  • instead of visiting of node of the graph to check if it has an edge to the node to delete, it is more efficient to only visit the nodes given in the adjacency list of node u.
  • you don't need recursive function to remove the edge (u,v). Instead:
    text prev=NULL; for (p=G->adj[v]; p && p->key!=u; prev=p, p=p->next); if (prev==NULL) G->adj[v]=p->next; else prev->next = p->next;

  • you could perhaps try memmove() to shift the G->adj array. This should be equivalent to your shift operation, but eventually memmove() implementation is faster, because the libc designers usually take advantage of the underlying hardware (e.g. SSE instructions).
    text if (u!=G->nv-1) memmove(G->adj, G->adj[u+1], (G->nv-u-1)*sizeof(edge*));

I don't know however if your data structure is the most suited for what you're targeting, but perhaps can Mrs. Google help you further...

HTH, Lo�c