10
\$\begingroup\$

The code is an implementation of the Gauss–Seidel method. I would like a general review of the code.

PS: I use code::blocks IDE

//generalized code for gauss seidal iteration method 
#include<stdio.h>
#include<malloc.h>

#define chk_end 0.01

int row_lim=0,col_lim=0;

float** partial_pivot(float** mat,int row,int col);//finds the pivot of matrix
int condition(float* result,float* temp,int row);//checks the terminating condition
float* final_result(float** mat,float* result,int row,int col);//calculates final result
float** create_mat(float** mat,int row,int col);//creates a matrix of row X col
float** feed_mat(float** mat,int row,int col);//inputs value in matrix
void disp_mat(float** mat,int row,int col);//displays matrix

float** partial_pivot(float** mat,int row,int col)
{
 float pivot;
 int i=0,j=0,k=0,l=0;

 if(row_lim<row)
 {
  for(i=0,j=col_lim;i<row-1;i++)
  {
   for(l=row_lim;l<row-i-1;l++)
   {
   if(mat[l][j]<mat[l+1][j])
   {
    for(k=0;k<col;k++)
    {
     pivot=mat[l+1][k];
     mat[l+1][k]=mat[l][k];
     mat[l][k]=pivot;
    }
   }
   }
  }
 row_lim++;
 col_lim++;
 partial_pivot(mat,row,col);
 }

 return mat;
}

int condition(float* result,float* temp,int row)
{
 int i=0,flag = 0;

 for(i=0;i<row;i++)
 {
 if(result[i] > temp[i])
 {
  if((result[i]-temp[i]) < chk_end)
  {
   flag = 1;
  }
  else
  {
   flag = 0;
   break;
  }
 }

 else
 {
  if((temp[i]-result[i]) < chk_end)
  {
   flag = 1;
  }
  else
  {
   flag = 0;
   break;
  }
 }
 }

 return flag;
}

float* final_result(float** mat,float* result,int row,int col)
{
 int k=0,i=0,j=0;
 float T = 0;
 float *temp;

 temp=(float*)malloc((row)*sizeof(float));//Edit: row-1 changed to row
 for(i=0;i<row;i++)
 {
  temp[i] = 0;
 }

 printf("Processing result ... \n");


 do
 { 
   for(k=0;k<row;k++)
   temp[i] = result[i];    //was a logical error, sorry
   {
   for(i=0;i<row;i++)
   {
    for(j=0;j<(col-1);j++)
    {
     if(i==j)
     {

     }
     else
     {
      T += (mat[i][j])*(temp[j]);
     }
    }
    result[i] = ((mat[i][col-1] - T)/(mat[i][i]));
    //temp[i] = result[i];
    T=0;
   }
  }
 }
 while(condition(result,temp,row) == 0);

 free(temp);

 return result;
}

float** create_mat(float** mat,int row,int col)
{
    int i=0;

     mat=(float**)malloc((row)*sizeof(float*));//Edit: row-1 changed to row
     if(mat)
     {
        for(i=0;i<row;i++)
        {
            mat[i]=(float*)malloc((col)*sizeof(float));//Edit: col-1 changed to col
        }
     }

    return mat;
}

float** feed_mat(float** mat,int row,int col)
{
    int i=0,j=0;

    for(i=0;i<row;i++)
    {
        for(j=0;j<col;j++)
        {
            printf("Enter mat[%d][%d] element:\n",i,j);
            scanf("%f",&mat[i][j]);
        }
    }

    return mat;
}

void disp_mat(float** mat,int row,int col)
{
    int i=0,j=0;

     for(i=0;i<row;i++)
     {
        for(j=0;j<col;j++)
        {
            printf("\t%f",mat[i][j]);
        }
        printf("\n");
     }

}

int main()
{
 float **mat,*result;
 int i=0,row=0,col=0;

 mat=NULL;

 printf("Enter the number of Equations :\n");
 scanf("%d",&row);
 col=row+1;

 result = (float*)malloc((row)*sizeof(float));//Edit: row-1 changed to row
 for(i=0;i<row;i++)
 {
  result[i] = 0;
 }

 mat=create_mat(mat,row,col);

 printf("Enter the values in the %d x %d matrix:\n",row,col);
 mat=feed_mat(mat,row,col);

 printf("matrix entered by you:\n");
 disp_mat(mat,row,col);

 mat=partial_pivot(mat,row,col);

 printf("pivoted matrix entered:\n");
 disp_mat(mat,row,col);

 result = final_result(mat,result,row,col);

 printf("\nResult is : \n");
 for(i=0;i<row;i++)
 {
  printf("  x[%d]   %f",i,result[i]);
 }

 for(i=0;i<row;i++)
 {
     free(mat[i]);
 }
 free(mat);

 free(result);

 return 0;
}
\$\endgroup\$
6
\$\begingroup\$

A word of warning: I am very much a C newbie myself, so it may be good to take my advice with a grain of salt.

Formatting

The only good thing I can say about your formatting is that it's consistent. But a 1-space indent really isn't readable. I recommend 4 spaces instead. Please put some whitespace around your operators: a space after every comma, a space on each side of =, -, +, *, …. Don't cram multiple assignments on one line: I find

int i, j, k;

or

int i = 0;
int j = 0;
int k = 0;

acceptable, but not int i=0,j=0,k=0;.

If you are targetting a C89-compatible environment, this is as far as we can go. I recommend you move on (e.g. to C++ or C99), where we can put the declaration of for-loop variables inside the loop itself:

for (int i = 0; i < whatever; i++)

… and we also don't have to put all declarations at the top of each scope – just declare your variables at the point of first use.

malloc and error handling

Always check the return value of malloc:

float *temp = (float*) malloc(row * sizeof(float));
if (temp == NULL)
{
    /* return some error code */
}

C isn't object oriented, but whatever

You always bundle the three variables mat, row, col. I suggest you put those into a struct, and pass this single struct around instead:

typedef struct {
    float **mat;
    int rows;
    int cols;
} Matrix;

I would then create a Matrix_new function that creates and populates an instance, e.g.

// this code assumes C99. Ignore my more compact brace style.

/**
 * Allocates a new `Matrix` instance. Returns a pointer to the instance on
 * success, a `NULL` pointer on failure.
 *
 * A Matrix must always be freed via `Matrix_destroy(instance)`!
 */

Matrix*
Matrix_new(int rows, int cols) {

    Matrix* matrix = (Matrix*) malloc(sizeof(Matrix));
    if (matrix == NULL) {
        return NULL;
    }

    float** mat = (float**) malloc(rows * sizeof(float*));
    if (mat == NULL) {
        free(matrix);
        return NULL;
    }

    for (int i = 0; i < rows; i++) {
        float* row = (float*) malloc(cols * sizeof(float));
        if (row == NULL) {
            // if a row allocation fails, we have to `free` the previous rows.
            i--;
            for (; i >= 0; i--) {
                free(mat[i]);
            }
            free(mat);
            free(matrix);
            return NULL;
        }

    }

    matrix->mat  = mat;
    matrix->rows = rows;
    matrix->cols = cols;
    return matrix;
}

and of course, a mirroring Matrix_destroy:

/**
 * Safely frees a Matrix instance.
 */

int
Matrix_destroy(Matrix* matrix) {

    if (matrix == NULL) {
        return 0;
    }

    float** mat = matrix->mat;
    int rows = matrix->rows;

    for (int i = 0; i < rows; i++) {
        free(mat[i]);
    }

    free(mat);
    free(matrix);

    return 1;
}

Then, you can rewrite your other methods to take advantage of this bundling.

Return values

Very often (e.g. in final_result, create_mat, feed_mat), you return a pointer from your function even when that pointer was an argument to that function. It is generally preferable to return an error code, and to return values via pointers/out-arguments.

For example, final_result could be changed to:

/**
 * Calculates the final result.
 *
 * Returns an error code.
 *
 * After successful execution, the `out_result` pointer will point to the 
 * result, which is a `float*` of `self->rows` length. The caller owns the
 * result, and is responsible for calling `free(*out_result)`.
 *
 * Example usage:
 *
 *     float *result;
 *     if (! final_result(matrix, &result)) {
 *         // abort with error
 *     }
 *     // do something with `result`:
 *     for (int i = 0; i < matrix->rows; i++) {
 *         printf("%f\n", result[i]);
 *     }
 *     free(result);
 */

int
final_result(Matrix* self, float** out_result) {
    if (self == NULL) {
        return 0;
    }

    float* temp = (float*) malloc(self->rows * sizeof(float));
    if (temp == NULL) {
        return 0;
    }

    float* result = (float*) malloc(self->rows * sizeof(float));
    if (result == NULL) {
        free(temp);
        return 0;
    }

    for (int i = 0; i < self->rows; i++) {
        temp[i]   = 0;
        result[i] = 0;
    }

    printf("Processing result...\n");

    do {
        for (int i = 0; i < self->rows; i++) {
            float T = 0;
            for (int j = 0; j < self->cols - 1; j++) {
                if (i != j) {
                    T += self->mat[i][j] * temp[j];
                }
            }
            result[i] = (self->mat[i][self->cols - 1] - T) / self->mat[i][i];
            temp[i] = result[i];
        }
    } while (condition(result, temp, self->rows) == 0);

    free(temp);
    *out_result = result;
    return 1;
}

Miscellaneous Helper Functions

memset

When zeroing all elements in some data structure, you can often use memset from string.h instead:

temp=(float*)malloc((row)*sizeof(float));//Edit: row-1 changed to row
for(i=0;i<row;i++)
{
 temp[i] = 0;
}

becomes

float* temp = (float*) malloc(row * sizeof(float));
if (temp == NULL) {
    // return error code
}
memset(temp, 0, row * sizeof(float));

fabsf

If you are interested in an absolute value of a float, use the fabsf function from math.h. With it, your condition function simplifies to:

int
condition(float* result, float* temp, int row) {
    int flag = 0;

    for (int i = 0; i < row; i++) {
        if (fabsf(temp[i] - result[i]) < chk_end) {
            flag = 1;
        }
        else {
            flag = 0;
            break;
        }
    }

    return flag;
}
\$\endgroup\$
  • \$\begingroup\$ dude heartly thanks a highly detailed way of making my program optimized and thanks for spending time on it,, i am having one doubt on your point please just elaborate what you want to say through it i.e. /*Return values Very often (e.g. in final_result, create_mat, feed_mat), you return a pointer from your function even when that pointer was an argument to that function. It is generally preferable to return an error code, and to return values via pointers/out-arguments. */ //what do you mean by return values via pointers/out-arguments// \$\endgroup\$ – Amitesh Mar 25 '14 at 3:03
  • \$\begingroup\$ @Amitesh I added an example rewrite of final_result that showcases this: The return value is a status code, whereas the result is returned via a pointer argument. In this simple case, this isn't really necessary, but quite often you'll want to return multiple values from a function: pointer arguments are the best way to do that. \$\endgroup\$ – amon Mar 25 '14 at 10:13
  • \$\begingroup\$ thanks dude got your point and will optimize my code by using your tips thanks. \$\endgroup\$ – Amitesh Mar 25 '14 at 15:09

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.