Using a greedy algorithm to find the best path in a 2D array

I'm a student and this is one of my assignments. My professor basically confirmed that my code is correct but all he grades on is if I finished the assignment correctly but not the coding style. I think I did poorly in terms of code style and implementation. I'm very frustrated and would like some guidance on how to improve and think about problems in a better way.

```
#include
#include
#include
#include

using std::cout;
using std::cin;
using std::ifstream;
using std::endl;

const int MAX_ROW = 100;
const int MAX_COL = 100;

//Function Prototypes
void mapData(int Map[100][100], int rowCount, int colCount);
int findMax(int Map[100][100], int rowCount, int colCount);
int findMin(int Map[100][100], int rowCount, int colCount);
void drawMap(int Map[100][100], int rowCount, int colCount);
int drawLowestElevPath(int Map[100][100], int rowCount, int colCount, int startingRow);

int main() {
int Map[100][100];
int startingRow, max, min, elevChange;

mapData(Map, MAX_ROW, MAX_COL);
max = findMax(Map, MAX_ROW, MAX_COL);
min = findMin(Map, MAX_ROW, MAX_COL);
drawMap(Map, MAX_ROW, MAX_COL);

// Taking the starting row input from the user
cout > startingRow;

while (true)
{
if (startingRow 99) {
cout > startingRow;
} else {
break;
}
}

elevChange = drawLowestElevPath(Map, MAX_ROW, MAX_COL, startingRow);

return 0;

}

// Inputs a list of integers from a text file into a 2D array in row major
void mapData(int Map[100][100], int rowCount, int colCount)
{
ifstream myIn;
myIn.open("mapdata.txt"); //opening the text file

for (int i = 0; i > Map[i][j]; //reading each integer into an index in the array
}
}
}

// Determines the largest integer in the array
int findMax(int Map[100][100], int rowCount, int colCount)
{
int max = Map[0][0];
for (int i = 0; i max)
{
max = Map[i][j];
}
}
}

return m

Here is a short write-up of things I noticed while reading the code. Please excuse the missing structure, I might restructure it properly later. Please ask if the reasoning behind points is unclear.

-
Instead of build-in arrays use std::array. Then you will not need to pass rowCount and colCount. It is also much safer, because you cannot pass std::arrays with wrong sizes.

-
Use MAX_ROW and MAX_COL everywhere where they are actually meant, i.e. also in Map[100][100], so that you can change the value without having to check your code correctness afterwards.

-
If your functions are supposed to take arrays of varying sizes, than you should pass as int** Map instead of int Map[100][100]. If only arrays of dimensions MAX_ROWxMAX_COL are supposed to be allowed, then it is unnecessary to pass rowCount and colCount because they are required to be MAX_ROW and MAX_COL.

-
Don't declare all local variables at the beginning of functions. That is only (somewhat) required in old versions of C and has never been required in C++. Declare variables at the point where they are initialized or first used and in the narrowest scope that they are required. For example:

int max = findMax(Map, MAX_ROW, MAX_COL);

There is no reason to have int max; declared beforehand. It is just more code and risking usage of max before initialization.

Another example:

int elevChange2 = abs(Map[currentRow][currentCol] - Map[currentRow][currentCol + 1]);

elevChange2 needs not be retained outside the if-block. Therefore its scope should also be limited to it, making the code more readable, reducing probability of unintended misuse (equal names, etc.) outside the intended scope and allowing for easier optimization by the compiler.

-
Move the file name mapdata.txt either to a constant global string or better read it from argv and pass it to mapData. This might be beyond your intended scope, because you don't want the file name to change, but hardcoded file names are usually problematic (need to recompile just to change some file names).

-
I think the name mapData is not really saying what the function does readMap or similar seems more appropiate.

-
ifstream has a constructor from file names. That means that you can write

ifstream myIn("mapdata.txt");

with the same effect, reducing code and making it impossible to use myIn before actually opening the file.

-
myIn is the only variable you prefix with my. That is inconsitent. Use a better name like inFile or something.

-
Using output parameters is not really good C++ style if return values could be used instead. If you used std::array as I mentioned, then you could create the array in mapData and return it after filling with the file data instead of passing an array pointer from the caller.

-
Your loop in main could be written shorter. You use an endless loop while(true) which runs until a break; statement. The break; statement happens exactly if the condition startingRow 99 is not satisfied. Or expressed a bit differently, the loop continues as long as startingRow 99. Therefore you can just write (and much more readable):

while(startingRow  99)
{
    cout > startingRow;
}

-
Use while(startingRow = MAX_ROW) for the same reason as 2. because otherwise changing MAX_ROW will break your code unless you can find all numbers actually referring to it semantically.

-
You might want to have error handling in your input code. As is trying to input a string instead of a valid number during cin >> startingRow; will result in an uncaught exception crashing your program. If you haven't yet learned about exception you probably shouldn't bother yet, but otherwise put a try/catch block arround it and give proper error messages.

-
elevChange = drawLowestElevPath(Map, MAX_ROW, MAX_COL, startingRow);: Here you are setting elevChange, but it is not actually used afterwards anymore. That is unnecessary. In fact that is the only use of elevChange, so it is not needed at all.

-
In drawLowestElevPath you should not reuse Map to draw the path. The caller passes Map into the function and the function is modifying it. I don't think this is expected. Instead create a new int[MAX_ROW][MAX_COL] and use it to write to, leaving Map unmodified.

-
In drawLowestElevPath you are repeating code unnecessarily. For example Map[currentRow][currentCol] = 1; will be executed no matter which if/else branch is executed. Therefore you could simply put it at the end of the for-block (at least if you followed my point 14., otherwise you might have to restructure your code, i.e. first calculate all elevChanges allowed, then set =1 and then check the next step).

Another example is currentCol++ which is repeated in every branch. Put it at the end of the for-block once instead.

-
I think you did not properly explain what path your code is supposed to find. Is the gr

StackExchange Code Review Q#142427, answer score: 15