current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am writing a program for learning purposes that takes an input of a file structured similarly to:

13,22,13,14,31,22, 3, 1,12,10
11, 4,23, 7, 5, 1, 9,33,11,10
40,19,17,23, 2,43,35,21, 4,34
30,25,16,12,11, 9,87,45, 3, 1
1,2,3,4,5,6,7,8,9,10

Which will then read the values from the file, figure out and print the largest sum you can make from the digits on each line which is 50 or less. (Also can not be the same number used twice). I feel I have come to an okay solution but there are a few issues. I want to try and separate the code into separate functions and currently if the text file inputted ends with a newline char the program loops once too far and hence prints out the calculation for a non existent line, I'm not sure how to get around this.

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
  FILE *fp;
  int hv, lv,line,i, linecount,val[5][10]; //hv + lv = the two highest values on a line, line=sum of hv and lv.
  int j=0;

  // test file succesfully opened
  if((fp=fopen(argv[1], "r")) == NULL){
    printf("Cannot open file.\n");
    exit(1);
  }

  //Store values in 2d array
  for(i=0;!feof(fp);i++){
    while(j<10){
      fscanf(fp, "%d,",&val[i][j++]);
    }
  j=0;
  }
  linecount=i-1; //set linecoutn to No of lines

//test and print result
 for(i=0; i<=linecount; i++){ // for each line
   hv=0, lv=0, line=0; //reset values for each line
   for(j=0;j<10;j++){ // for each value
     for(int a =0; a<10; a++) { //test against all in line
       if(a!=j && (val[i][j]+val[i][a]<=50)){ //if two values arent equal and sum is less than 50
     if((val[i][j]+val[i][a])>line){ //if value is greater than previous value
       hv=val[i][j];
       lv=val[i][a];
       line=hv+lv;
     }
       }
     }
   }
   printf("Line %d: largest pair is %d and %d, with a total of %d\n", i+1, hv, lv, line);
 }

 fclose(fp);
}

So what I'm looking for is code optimization advice and help

Thanks

share|improve this question

1 Answer 1

up vote 0 down vote accepted

Brad, you code compiles cleanly, which is a good sign :-) Clearly it could be split into functions. An obvious approach would be to split the reading of the values from the processing of those values.

I would take a slightly different approach, but remember there is no one "correct" solution. I would read and process the file one line at time. So the pseudo code would be:

    for each line
        read the line data  (function)
        find greatest sum in line  (function)
        store sum if bigger than max value found

This approach avoids the need for a multi-dimensional array and means you can handle an arbitrary number of lines.

Note that nested loops are often not desirable, but are sometime necessary. Finding the greatest sum in the line looks like a case where a nested loop is optimal. But this double loop should be a function.

Also check the return value from fscanf. If you do this you can avoid processing a blank line.

A few comments on notation:

  • Be consistent!!! This is important although it might seem trivial. Many people reading your code and seeing some places where a comma is followed by a space (preferred) and some where it is not will judge the code badly. Similarly, comments should be consistent - follow // by a space.

  • Defining just one variable per line is usually preferred.

  • Leave a space after keywords (if, else, for, while etc). Similarly, leave a space before and after = (or != etc). (these are my preferences, others might differ).

  • Many people, including me, prefer code and comments to extend to no more than 80 columns.

  • Send error messages to stderr and on error from system and library calls that set errno (the global error variable) use perror (which prints to stderr).

  • Exit on failure with exit(EXIT_FAILURE);

  • On completion, return a value from main, normally return EXIT_SUCCESS

  • Oh and finally, delete the useless comments. Nearly all of your comments are worse than pointless - they add noise, not information.

share|improve this answer
    
This has been really great help in cleaning up and organising my code. It's a lot more legible now. Thanks! –  BradStevenson Nov 10 '12 at 16:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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