Find the matrix row having the largest sum

Question

void largest_row(void)
{
 int largest_row, temp = 0, sum = 0;
 for (int i = 0; i < row_count; i++)
 {
 for (int j = 0; j < row_count; j++)
 {
 sum += array[i][j]; //Array referenced here is globally defined
 if (sum > temp)
 {
 largest_row = i;
 }
 temp = sum;
 }
 }
 printf("%i\n", largest_row);
 return;
}

The function largest_row() prints the index number of the row in a 2d array having the largest sum.

In what areas could my code or algorithm be improved (e.g. a recursive implementation)? I think there is a better way, and I want to get exposed to well-designed code and more efficient algorithms as I continue to learn to program.

As of now, I think I depend too much on loops and that limits my programming experience. Is it even right to think that way? For reference, and if you guys are familiar, I'm currently in week 3 of CS50x (infamous Tideman problem). I'm almost done with it but I want my code to be better. That's the goal of my question at least if any of these is too vague already.

• Pavlo Slavynskyy: The code doesn't work as intended. If the array is {{2,2},{1,1}}, it says row 1 ({1,1}) is the largest.
• Toby Speight: Next time you post, you can make life easier for reviewers by showing a complete program (with all the necessary #include lines, variable definitions, and a main() that exercises the function). Don't edit the code now that you have answers here, but consider that for future review requests, as that will likely attract better responses.
• user3629249: this site is for code review of working code. The posted code does not compile! and has a logic flaw.

Answer 1

Unless we're told the array is a square array, we have a bug. I'm guessing that should be j < column_count (or whatever the appropriate global is called, since you haven't shown the declarations).

We have another bug where we update the largest_row before we have finished adding values from the row. It's possible that the rest of the elements are sufficiently negative that this isn't the largest row so far, after all - leave the sum > temp test until the j loop is finished.

It would be better to accept the array and dimensions as arguments, and return the result, rather than reading from global variables and writing output directly. For larger programs, we tend to want functions with a single responsibility, so that we can compose programs from the individual pieces.

Answer 2

Some guess

The code you provide is missing important information that we must guess.

Being a programmer means being able to present the problem and the solution without ambiguity. Here is one important and some pedantic ambiguities listed.

• What type are the items referenced by array?

  We can assume they are int but depending on the compiler settings your code could still work for other types. The problem may be hidden behind warnings that we can not see.

  If for example the array contains unsigned int then adding what is a positive value (in the array) can be subtracted if the top bit is 1

• What is row_count, is it a static typed value, a defined value (has no type), or ...?

• You provided no library references, eg #include <stdio.h> Yes it is a very standard include associated with printf but we are still left to guess this is a fact.

Rewrite

Taking your code at face value

Two bugs

1. Sum will sum all rows as you do not reset its value for each row.

2. The max value temp is initialized with the wrong value. It should have the lowest possible value. That means that largest_row may never get a value.

   Depending on the compiler setting what is printed could be a random value, or zero.

Poor design

The code is inefficient and the naming is poor.

• largest_row is a function so give it a capital Largest_row

• largest_row is not what the function does. It prints the index of the largest row. Using PrintIdxLargestRowSum would be better however it is too long. If we make the function return the largest row, the name could be IdxLargestSum

• It is a little odd as you are checking for a max value as you are summing the row.

  First sum the row, then check for the max value.

• Keep variables scoped as close as possible to the code they are used in. Declare sum inside the first loop.

Using your code as a template we get

// Assumes global row_count > 0
int Idx_largest_row(void) {
 int idx, max;
 for (int i = 0; i < row_count; i++) {
 int sum = 0
 for (int j = 0; j < row_count; j++) {
 sum += array[i][j]; 
 }
 if (i == 0 || (i > 0 && sum > max)) {
 max = sum;
 idx = i;
 }
 }
 return idx;
}
// use as
printf("%i\n", Idx_largest_row());

Example

The above rewrite is still rather poor in my view.

• There should be two functions, one to sum a row the other to find the max sum.

• The array and row counts should be an argument

• The inner arrays should be of defined length

#include <stdio.h>
#define COL_COUNT 5
#define uint unsigned int
int rows[][COL_COUNT] = {
 {10, 2, 320, 344, -130},
 {30, 12, 330, 444, -160},
 {100, 2, 123, 207, 50},
 {102, 82, 530, 544, -150},
 {103, 72, 730, 444, -140}
};
int SumRow(int *row, uint count) {
 int sum = 0;
 for (uint i = 0; i < count; i++) { sum += row[i]; }
 return sum;
}
uint LargestRowIdx(int rows[][COL_COUNT], uint rowCount) {
 uint idx = 0;
 int max = SumRow(rows[0], COL_COUNT);
 for (uint i = 1; i < rowCount; i++) {
 int sum = SumRow(rows[i], COL_COUNT);
 if (sum > max) { 
 max = sum; 
 idx = i;
 }
 }
 return idx;
}
void main() {
 printf("Index of largest sum: %u\n", LargestRowIdx(rows, 5));
 return;
}

_{Note that the number of rows is a magic number 5, however you should use a defined value or variable}

_{Note I use unsigned int when indexing and rather than have to type all that I use the the defined #define uint unsigned int. There is no reason you can not use an int for indexes}

_{Note I use camelCase names rather than snake_case. Which to use is up to you}

• Toby Speight: Why do you use PascalCase for your function names? That's very unidiomatic in C.
• Wör Du Schnaffzig: You may use ìnt max = INT_MIN` from limits.h and then start iteration on index 0.
• Blindman67: @TobySpeight I find the more compact camelCase and PascalCase easier to read and more congruent with modern languages. Idiomatic C is very old school and I feel without people actively pushing a change, nothing will change. Note I did add a note about name style at bottom of answer.,
• Wör Du Schnaffzig: @Blindman: The library is just a header file and contains compile time macro definitions. Your initialization of max and sum is an unnecessary code repetition and conflicts with D.R.Y. paradigm (don't repeat yourself)
• L. F.: #define is actively discouraged over alternatives such as typedef, so it isn't really "safer". It is generally recommended to avoid macro names whenever possible (in this case, avoiding uint and writing unsigned directly might be preferred).