Swapping 2 variables from a matrix in C

Question

So I have to make a program that can determinate the maximum from the diagonal of a matrix. Than I have to swap it's place of the first variable of the matrix (a[1][1]) with the maximum of that matrix, the other elements beeing unchanged.

Here is my code:

#include <stdio.h>
#include <conio.h>
#include <ctype.h>
#include <string.h>
#include <math.h>
#include <stdlib.h>

int a[20][20];
int main(){
    int i, j, n1, n2, max;
    printf ("\nIntroduceti numarul de coloane pentru matricea A: ");
    scanf (" %d", &n2);
    printf ("\nIntroduceti numarul de randuri pentru matricea A: ");
    scanf (" %d", &n1);
    printf("\nIntroduceti elementele primei matrice: ");
    for(i=1;i<=n1;i++){
        for(j=1;j<=n2;j++){
            printf("\na[%d][%d] = ", i, j);
            scanf("%d",&a[i][j]);
        }
    }
    printf("\nMatricea A este:\n");
    for(i=1;i<=n1;i++){
        printf("\n");
        for(j=1;j<=n2;j++){
            printf("%d\t",a[i][j]);
        }
    }


    do {
        for(i=1;i<=n1;i++){
            if(a[i][i]>max) {
                max=a[i][i];
            }
        }
    } while (i<=n1);
    printf ("\nMaximul de pe diagonala este: %d", max);

    a[1][1]=a[1][1]^max;
    max=max^a[1][1];
    a[1][1]=max^a[1][1];

        for(i=1;i<=n1;i++){
        printf("\n");
        for(j=1;j<=n2;j++){
            printf("%d\t",a[i][j]);
        }
    }

    return 0;
}

What I did wrong? My program swaps only the maximum with the first variable a[1][1], but forgets to place a[1][1] where the maximum is.

Answer 1

You iterate completely through the diagonal elements, keeping track of only the maximum value, then (after the loop) you try to swap this value with the first element of the matrix. The problem is that you no longer know where the maximum value was, since you never recorded that! You just record what the value was (in max), and ignored its index in the matrix.

For example, suppose your maximum value happens to be at a[2][2]. You iterate through the diagonal elements, find the maximum value, store it in max. At the end of your loop, you can't swap a[2][2] with a[0][0] because you lost the information that the maximum is at a[2][2]. (Assuming you learned the issue about array indexes starting at 0.) What you are doing is writing max to the first element a[0][0]. a[2][2] is unchanged because you don't change it, and you forgot where it was to change it anyway.

You need to keep track of the maximum value in some variable (max as it is written now is fine) and add another variable (say, max_pos for example) to keep track of the index of where this maximum value is. (You only need one int because they're diagonal elements, so both array indexes are always the same.) Any time you change the maximum value max, also update the maximum position max_pos. This way you would know at the end to swap a[0][0] and a[max_pos][max_pos].

---

A brief sketch of what the code might look like (I'll leave it you to fill in any details):

Add a declaration of a new variable to keep track of the position (say, max_pos) of the maximum value:

int i, j, n1, n2, max, max_pos;

You need to initialize max and max_pos with the first diagonal element, then iterate through the others.

max = a[0][0];
max_pos = 0;
for (i=0; i<n1; i++){
    if (a[i][i] > max) {
        max = a[i][i];
        max_pos = i;    // update max_pos anytime max is updated
    }
}

Notice how I changed the range of the indexes from 1...n1 to 0..n1-1. (Make this consistent with the rest of your code.) Array indexes in C start with 0, not with 1. Notice here that you don't need that do {} while () loop. Think about what it is really doing (hint: nothing).

Then, after this, you can swap a[0][0] and a[max_pos][max_pos]. By the way, XOR swap is not necessary and makes the code harder to read. Just swap values with a temporary variable. Modern compilers are very good at optimizing and can likely implement this with registers, so you're probably not gaining whatever efficiency you think you are (and you're only doing it once). Don't try to outsmart your compiler.

Answer 2

Suggestion for cleanup

When you have

int a[20][20];

The valid range of elements are a[0][0] - a[19][19].

You are accessing elements a[1][1] - a[n1][n2] in all the for loops since you are using:

for(i=1;i<=n1;i++){
    for(j=1;j<=n2;j++){

That won't be a problem unless the value of n1 or the value of n2 is 20.

Change those loops to use:

for(i=0; i < n1; i++){
    for(j=0; j < n2; j++){

Main problem

The main problem in your code is that you are using max before it's initialized. Add a line to initialize max to a[0][0] before the loop to compute its final value.

// Remove the `do-while` part. You don't need it.
max = a[0][0];
for(i=0; i < n1; i++){
    if(a[i][i]>max) {
        max=a[i][i];
    }
}