8
\$\begingroup\$

I wanted to write a transpose function for N*N mat without it getting literals or #define values and I want it to compile with gcc -ansi -pedantic -Wall -Werror

#include<stdio.h> /*printf */
/*function to transpose a N*N 2D mat */
void TransposeOf2DArray(int* mat, size_t n)
{
   int i = 0;
   int j = 0;

   if(NULL == mat)
   {
     return;
   }

   for(i= 0; i< n; ++i)
   {
     for(j = i ; j< n; ++j)
     {
       int temp = mat[(n*i)+j];
       mat[(n*i)+j] = mat[(n*j)+i];
       mat[(n*j)+i] = temp;
     }
  }
}
/*print function for int mat n*n */
void printMat(const int* mat, int n)
{
  int i = 0;

  if(NULL == mat)
  {
     return;
  }

  for(i = 0 ; i< n*n ;++i)
  {
     printf("%d| ", mat[i]);
     if((1+i)%n == 0)
     {
         printf("\n");
     }
  }
}
int main()
{
   int mat[][3] = {{0,1,2},{3,4,5},{6,7,8}};

   printf("Before transpose: \n");
   printMat((int*)mat, 3);
   TransposeOf2DArray((int*)mat, 3);
   printf("\nAfter transpose: \n");
   printMat((int*)mat, 3);


   return 0;
}
\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, Mast, Heslacher, Hosch250, Raystafarian Jan 8 at 5:35

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – πάντα ῥεῖ, Hosch250, Raystafarian
If this question can be reworded to fit the rules in the help center, please edit the question.

8
\$\begingroup\$

Here are some things that may help you improve your program.

Declare variables only where needed

Old-style C required all variables to be declared at the top of the function in which they were used, but modern C has not required this for many years. For that reason, you can remove the declarations of i and j and incorporate them into the for loops instead, as in the following suggestion. (Note that this requires C99 or later.)

Use size_t instead of int where appropriate

My version of gcc complains because size_t is unsigned and int is unsigned. To address that, we can change the types of i and j:

for (size_t i = 0; i < n; ++i) {
    for (size_t j = i; j < n; ++j) {

Think carefully about the algorithm

The diagonal of the matrix doesn't really need to be touched. This can easily be addressed by starting the inner loop from i + 1 instead of i.

Consider using pointers

It might be a bit more clear within the inner loop if pointers were used. Here's one way to do that:

int *a = &mat[(n*i)+j];
int *b = &mat[(n*j)+i];
// swap *a and *b
int temp = *a;
*a = *b;
*b = temp;

Consider adding testing

Since the transpose of a transpose of any matrix should equal itself, this suggests one method of testing the results. I'd suggest testing a few matrices with small size and manually worked answers and then a larger number of matrices with varying sizes using the double-transpose and checking for equality.

Omit return 0 in main

Since C99, the return 0 at the end of main is implicit and may be omitted.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

\$\endgroup\$
  • 4
    \$\begingroup\$ Omitting return 0 is an option, but one I generally disagree with - it's a quirk of the language that breaks uniformity, and may confuse beginners. \$\endgroup\$ – Reinderien Jan 6 at 15:25
  • 1
    \$\begingroup\$ De gustibus non est disputandum. Whether one prefers to use it or not, it's useful for programmers to know of this provision in the standard. \$\endgroup\$ – Edward Jan 6 at 15:28
  • 1
    \$\begingroup\$ I'll agree with that. However, it's also important to note that your answer contains C99 features, but the OP wants ansi (C89). Whereas I think you should keep the C99 recommendations, you need to mention the version difference. \$\endgroup\$ – Reinderien Jan 6 at 15:32
  • \$\begingroup\$ @Edward thank you, as it is now- I want the strictness of 89, so the return and value declarations are not avoided. will change to size_t and try to improve the algorithm. I have made more tests, but I omitted them. I thought about making a static switch function, but the code as a hole is short so I wasn't sure if its appropriate. Thank you so much for your time and input! \$\endgroup\$ – H.cohen Jan 6 at 15:47
  • 1
    \$\begingroup\$ I've updated my answers to point out which features requires C99 or later. It would be a shame to ignore the last 20 years of language evolution, but if one is restricted to only the 30-year-old language version, it should now be clear which features that affects. \$\endgroup\$ – Edward Jan 6 at 15:49
7
\$\begingroup\$

Reconsider ansi

-ansi is equivalent to C89, which is many versions behind the current standard (C99 -> C11 -> C17). C99 is popular and will buy you some great language features. C17 is supported by gcc, though, so you should use that.

Clean up your whitespace

You should add one or two blank lines between each of your functions.

Your tabs are non-uniform - they seem to vary between two and three spaces. Generally 3-4 spaces is standard; choose a standard and apply it with an IDE or advanced text editor.

Use const

printMat does not modify mat, so declare it const.

Don't double-initialize

@Edward correctly indicated that variable declarations should be pulled into the loop. One other thing: you initialize i=0 twice, so the first one has no effect. The j=0 will also have no effect. Avoid doing effect-less assignment.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your time and input, I will add lines, my white spaces shifted when I moved my code to this platform, I'll be sure to pay more attention to it next time. the const for the print function- is it not enough to state the const int* mat? Would Also want to know about the var initiation - in 89 they must be declared as I did, but I was led to believe it is good practice to init them at declaration - is it not? \$\endgroup\$ – H.cohen Jan 6 at 16:01
  • 2
    \$\begingroup\$ Yes - const int *mat will do. As for combined declaration and initialization - yes, this is generally a good idea (if you're in C99 and you can postpone declaration). If you're stuck in C89, it still seems like nicer form to only initialize your loop variables once you get to the beginning of the for. \$\endgroup\$ – Reinderien Jan 6 at 16:27
1
\$\begingroup\$

2D array or not?

A "2D array" in common parlance is loosely something like the following

int a[4][5];
int **b;
int *c[x];

For me, I prefer the only calling a a 2D array.

Yet I would not call int* mat a 2D array. It is a pointer and code-wise, used as a pointer to a single dimension array of int.

/*function to transpose a N*N 2D mat */      // ??
void TransposeOf2DArray(int* mat, size_t n)  // `int*` is a 1D

As code called the function with a cast implies something1 unnecessary is happening.

printMat((int*)mat, 3);

Consider

printMat(&mat[0][0], 3);
TransposeOf2DArray(&mat[0][0], 3);
// or 
printMat(mat[0], 3);
TransposeOf2DArray(mat[0], 3);

And re-word void TransposeOf2DArray() description.

White space before '\n'

Trailing white-space (not '\n') at the end of the line, too often causes problems. Consider avoiding that.

Return value from print

Not too often code checks the return value of print, primarily to detect errors. Yet printMat() still could provide a useful return.

int printMat(const int* mat, size_t n) {
  retval = 0;
  if (NULL) {
    size_t nn = n*n;
    size_t i = 0;
    for(i = 0 ; i < nn; ++i) {
      const char *sep = ((1+i)%n) ? "| " : "\n";  // No WS before \n
      int r = printf("%d%s", mat[i], sep);
      if (r) {
        retval = r;
        // Perhaps break here
      }
    }
  }
  return retval;
}

1 Casting often indicate something amiss. Avoid it as able. I do find casting reluctantly needed in some printf() and some assignments between different types, but rarely with specified function argument.

\$\endgroup\$

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