current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I am trying to write 2 functions, one to read the matrix (2D array) and other one to print it out. So far I have:

/* Read a matrix: allocate space, read elements, return pointer. The
   number of rows and columns are given by the two arguments. */
double **read_matrix(int rows, int cols){

    double **mat = (double **) malloc(sizeof(double *)*rows);
    int i=0;
    for(i=0; i<rows; i++){
    /* Allocate array, store pointer  */
    mat[i] = (double *) malloc(sizeof(double)*cols); 
   //what to do after??

     return mat;
}

then the print matrix function, not sure if it is correct

 void print_matrix(int rows, int cols, double **mat){
  for(i=0; i<rows; i++){    /* Iterate of each row */
    for(j=0; j<cols; j++){  /* In each row, go over each col element  */
    printf("%f ",mat[i][j]); /* Print each row element */
}
}}

and here is the main function I am using to run:

#include <stdio.h>
#include <stdlib.h>


double **read_matrix(int rows, int cols);
void print_matrix(int rows, int cols, double **mat);
void free_matrix(int rows, double **mat);

int main(){

  double **matrix;
  int rows, cols;
  /* First matrix */
  printf("Matrix 1\n");
  printf("Enter # of rows and cols: ");
  scanf("%d %d",&rows,&cols);
  printf("Matrix, enter %d reals: ",rows*cols);
  matrix = read_matrix(rows,cols); 
  printf("Your Matrix\n");  /* Print the entered data */
  print_matrix(rows,cols,matrix);
  free_matrix(rows, matrix);   /* Free the matrix */

    return 0;}
share|improve this question

closed as off topic by Glenn Rogers, palacsint, mseancole, Jeff Vanzella, Trevor Pilley Oct 23 '12 at 17:53

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.
2  
Please note that as per the faq, any code should be working, and actual code from a project rather than pseudo-code or example code. –  Glenn Rogers Oct 23 '12 at 8:21
1  
Note that scanf returns the number of values it read. In scanf("%d %d",&rows,&cols); it should return 2. If it returns 0 or 1 then the user entered non-numeric data. Subsequent use of scanf to read numeric data will fail until the non-numeric data is read or flushed (see fpurge()). scanf can also return -1 if the input stream is closed. –  William Morris Oct 23 '12 at 18:09

2 Answers 2

up vote 3 down vote

It seems OK to me as far as it goes. A couple of suggestions though:

  1. read_matrix may be better split up into two functions, one to create it and the other to read the contents from the source, even if the create_matrix function is called from read_matrix - and you need to be aware of the possibility that the allocation may fail.
  2. On the other hand, you may wish to consider just allocating a single block of size rows*cols of memory, and indexing it via y*rows + x rather than allocating several smaller blocks.
  3. For 'neatness', you should consider using calloc( number_of_elements, sizeof (double) ) instead of malloc, just to give a (possibly) sensible initial value.
  4. print_matrix could do with some layout changes, such that each row is on a separate line, and the corresponding cells line up under each other, something like %10.3f (but would depend on the type of data expected).
share|improve this answer
up vote 1 down vote

Its fine for what it does.

But you should try and group all the relavant information into a single structure so that you pass it round as one item (this will prevent typing mistakes).

typedef struct TwoDArray
{
    int      cols;
    int      rows
    doubel** data;
} TwoDArray;

TwoDArray* read_TwoDArray(int rows, int cols);

Declare one variable per line:

int rows, cols;   /* Any industry style guide will force you to split
                     this over two lines
                   */

You are not being payed to conserve vertical space. You are being payed to make the code as readable and maintainable as possible.

You need to pick a brace style and be consistent. There are a couple of styles out there (I can't argue that any one is better than another). So just find a style you like but then by consistent when using it:

Stuff       /* Lined up Style */
{
    Plop
}

Stuff  {    /* K&R up Style */
    Plop
}

Stuff       /* indented Style (rarer than the other two) */
   {
   Plop
   }

The only thing I would complain about (and its not major) is neatness. Use more white space to break the code into sections. This makes it easier for the reader to group stuff together into logical blocks.

int main()
{
  double **matrix;
  int      row;
  int      cols;

  printf("Matrix 1\n");
  printf("Enter # of rows and cols: ");
  scanf("%d %d",&rows,&cols);


  printf("Matrix, enter %d reals: ",rows*cols);
  matrix = read_matrix(rows,cols); 


  printf("Your Matrix\n");
  print_matrix(rows,cols,matrix);


  free_matrix(rows, matrix);

  return 0;
}

Last thing is comments. The comments you add are useless. 

free_matrix(rows, matrix);   /* Free the matrix */

One could even argue worse then useless as the comments over time may stray from the code (or it takes extra work to keep the comments lined up with the work).

The code should explain itself (as it does: the function free_matrix() does not really need any explanation). You should use comments to explain why (or potentially how).

/*
 * read_matrix()
 *    Parameters:
 *       rows/cols    the size of the matrix to be allocated.
 *    Returns:
 *        A pointer to an a data structure such that 
 *        the expression M[a][b] to be valid.
 *        Where (0 <= a < rows)  && (0 <= b < cols)
 *
 *    This structure should be released using the function free_matrix()
 */
double **read_matrix(int rows, int cols){
}
share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.