I'm not understanding why this code in C doesn't give me an output other than the first printf, and I would like some help. I've already tried swapping the names to input and read the file where I'm getting the matrix but it hasn't worked, Thanks in advance! Let me know if you think I can optimize something inside this code, I will need it very soon for an exam.
#include <stdio.h>
#include <stdlib.h>
#define MIN_DIMENSION 5
#define MAX_DIMENSION 10
#define FILE_NAME "inputquattro.txt"
#include <stdio.h>
#include <stdlib.h>
#define MIN_DIMENSION 5
#define MAX_DIMENSION 10
typedef struct {
char *inputquattro;
int N;
int M;
} InputParams;
InputParams readInput(int argc, char *argv[]) {
InputParams params;
if (argc != 4) {
fprintf(stderr, "Usage: %s inputquattro.txt N M\n", argv[0]);
exit(1);
}
params.inputquattro = argv[1];
params.N = atoi(argv[2]);
params.M = atoi(argv[3]);
if (params.N < MIN_DIMENSION || params.N > MAX_DIMENSION || params.M < MIN_DIMENSION || params.M > MAX_DIMENSION) {
fprintf(stderr, "Invalid dimensions. Be sure N and M are between %d and %d\n", MIN_DIMENSION, MAX_DIMENSION);
exit(1);
}
return params;
}
double **allocateMatrix(int N, int M) {
double **matrix = malloc(N * sizeof(double *));
if (matrix == NULL) {
fprintf(stderr, "Error during matrix memory allocation.\n");
exit(1);
}
for (int i = 0; i < N; i++) {
matrix[i] = malloc(M * sizeof(double));
if (matrix[i] == NULL) {
fprintf(stderr, "Errore during matrix memory allocation.\n");
exit(1);
}
}
return matrix;
}
void fillMatrix(InputParams params, double **matrix) {
FILE *file = fopen(params.inputquattro, "r");
if (file == NULL) {
fprintf(stderr, "Impossible file opening. %s\n", params.inputquattro);
exit(1);
}
for (int i = 0; i < params.N; i++) {
for (int j = 0; j < params.M; j++) {
if (fscanf(file, "%lf", &matrix[i][j]) != 1) {
fprintf(stderr, "Reading file error.\n");
fclose(file);
exit(1);
}
}
}
fclose(file);
}
void printMatrix(int N, int M, double **matrix) {
for (int i = 0; i < N; i++) {
for (int j = 0; j < M; j++) {
printf("%lf ", matrix[i][j]);
}
printf("\n");
}
}
void normalize(int N, int M, double **A, double **B) {
double max_col[M];
for (int j = 0; j < M; j++) {
max_col[j] = A[0][j];
for (int i = 1; i < N; i++) {
if (A[i][j] > max_col[j]) {
max_col[j] = A[i][j];
}
}
}
for (int i = 0; i < N; i++) {
for (int j = 0; j < M; j++) {
B[i][j] = A[i][j] / max_col[j];
}
}
}
void insertionSort(double *arr, int size) {
for (int i = 1; i < size; i++) {
double key = arr[i];
int j = i - 1;
while (j >= 0 && arr[j] > key) {
arr[j + 1] = arr[j];
j = j - 1;
}
arr[j + 1] = key;
}
}
void sortMatrixCols(int N, int M, double **matrix) {
for (int j = 0; j < M; j++) {
double column[N];
for (int i = 0; i < N; i++) {
column[i] = matrix[i][j];
}
insertionSort(column, N);
for (int i = 0; i < N; i++) {
matrix[i][j] = column[i];
}
}
}
int main(int argc, char *argv[]) {
InputParams params = readInput(argc, argv);
double **A = allocateMatrix(params.N, params.M);
double **B = allocateMatrix(params.N, params.M);
fillMatrix(params, A);
printf("Matrix A:\n");
printMatrix(params.N, params.M, A);
normalize(params.N, params.M, A, B);
printf("\nMatrix B:\n");
printMatrix(params.N, params.M, B);
sortMatrixCols(params.N, params.M, B);
printf("\nMatrice B sorted by columns:\n");
printMatrix(params.N, params.M, B);
// Free parameters allocated memory
for (int i = 0; i < params.N; i++) {
free(A[i]);
free(B[i]);
}
free(A);
free(B);
return 0;
}
Considering the file now provided by the author in anoher answer
and the provided code --- with some minimum changes in order to not use VLA
It may or may not be ok.
the code considered
I am not sure I understand what you say.
about the original code
why this?
Considering the simple
and also this
This
InputParamsthing is not useful. Why use this delegation of the input parameters parsing to another function? Why use this at all? It is used only infillMatrixjust to send the file name in.use of VLA is problematic
This is not standard:
Why not use the standard and simple
since the difference is 40 bytes at most?
this is your data
So do as usual and write this way. Use prototypes as above, let the code for the functions after
mainor in a separate file. It will make your life easier. And the life of others that read your code also easier.where is the matrix?
It is clear that your code process a MxN array of doubles. But there is no such thing in your code. This is called abstraction and the one you use (almost none) is not good. You should have a
Matrixthing.where are the parameters?
You need the dimensions and the name of the input file. But in the code the dimensions come from the command line, but the data is in the file. And the file name is hardcoded after all. Makes little sense.
Why not pass just the file name in the command line, and inside the file put the dimensions in the first line? It is called encapsulation and SRP for
single responsibility principleand it is a valuable thing.avoid
double** allocateMatrix(int N, int M);On the same line, what you do need is to create a matrix based on data in a file. Build a model on that. As close as the model is from the problem the easier your code becomes to write, adapt and test. Return type** is problematic: you always need more information and there is no place for it. It is a pointer to a pointer. But can be a pointer to an array. Or an array of pointers to pointers. Use encapsulation and return a container.
why copy the data and then sort and copy back?
Insertion sort is an in_place algorithm. It is very strange to extract the data and then copy it back as in
Also note that you do not need all those braces.
But just sort the data where it is.
a possibly better approach
Here follows an example of what I am saying, and some arguments. I just copied your code into this model. It is a long post just because I am coding along with the text.
a
MatrixthingAs you see, the dimensions are now part of the model, so we can redefine the operations using
a prototype
v1.hheaderso_filldoes just that and returns a pointer to the matrix. Encapsulation. Sure, as soon asso_fillgets the dimensions of the matrix it callsso_allocateto get a pointer to a new one. This is the single responsibility principle.so_printdoes the expected, but accepts now an optional message, very handy in testing.so_normalizereturns a normalize version of the inputMatrix.so_destroygets a pointer to aMatrixand erase it, freeing all memory and returningNULL. This is written now, as inRAIIinC++: as we write the constructor we write the destructor and test both.some code for these, in
v1.ca
mainto test thisIt does almost nothing, but we can run it already.
A matrix is created and then deleted.
output
It is good for moral support, at least ;).
a second step: building
allocateThis is simple: gets a file name, returns a
MatrixorNULL. And it is exactly what is in this example implementation:an example
i4.txtAnd we have already
print.a
mainfile for this testSo you see that is simpler to have
mainin a separate file. And using components makes it easier:so_fillcallsso_createto get an empty matrix with the exact dimensions.so_destroywas already written and tested.so_printeven accepts a mesage, so the code is easier to read.output
testing
normalizeandsortIt is convenient that normalize returns a new
Matrix. This way we can normalize the original matrix and sort and print the result by writingabout
sortNote that the code below is almost the same you wrote. but sorts the data inside the matrix
testing all functions
3rd test output
This is your code, just adapted to another structure. I am not focusing on the results and did not checked the algorithms. I Just wanted to show another way to write it, one that you can find more safe, readable and extensible. And it is a very common one.