Help me optimise my code!

This is a discussion on Help me optimise my code! within the C Programming forums, part of the General Programming Boards category; Hi everyone, as an aside to the sudoku thing i was trying, i wrote a program to calculate various aspects ...

  1. #1
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68

    Help me optimise my code!

    Hi everyone, as an aside to the sudoku thing i was trying, i wrote a program to calculate various aspects of a debt. (It was mainly an exercise in using functions)

    Anyway, it works and i think i'm finished but i'd like people to have a brief look at it. Since i'm teaching myself, i have no way of knowing if i'm doing something right. As we know, just cause it works doesn't mean its the best way!)

    Particularly the way i'm dealing with functions.

    Problem is the program is about 150 lines long. I don't know if thats big but i don't want to post a big program here. Would it be ok?

    I would really appreciate it if someone had a look cause i'm probably doing a lot of unnecessary things.

    Thanks a lot in advance.
    Kai.

  2. #2
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,897
    Yeah, you can post it. No harm in doing that. If people are interested, you'll get some feedback.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  3. #3
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Ok thanks!
    I hope people will have a brief look:
    Code:
    #include<stdio.h>
    
    float getbalance(void)
    	{
    	float balance;
    	printf("Enter balance.\n");
    	scanf("%f", &balance);
    	return(balance);
    	}
    
    float getrate(void)
    	{
    	float apr, rate;
    	printf("Enter the interest (APR) in percent.\n");
    	scanf("%f", &apr);
    	rate = apr / 1200;
    	return(rate);
    	}
    int getterm(void)
    	{
    	int term;
    	printf("Enter the term in months.\n");
    	scanf("%d", &term);
    	return(term);
    	}
    float getpayment(void)
    	{
    	float payment_fixed;
    	printf("Enter the payment amount ().\n");
    	scanf("%f", &payment_fixed);
    	return(payment_fixed);
    	}
    
    int iterate(float balance, float rate, float minpay, float payment_fixed, int silent)
    	{
    	float interest, payment;
    	float total_interest = 1;
    	int month = 1;
    	while (balance > 0)
    		{
    		interest = rate * balance;
    			if(interest <= 0.5)
    				if(rate == 0)
    					interest = 0;
    				else
    					interest = 0.5;
    		payment = ((minpay / 100) * balance) + payment_fixed;
    		balance = balance + interest;	
    			if(payment <= 5)
    				payment = 5;
    			if(balance <= payment)
    				payment = balance;
    		balance = balance - payment;	
    			if(silent != 0)
    				printf("%d\t%.2f\t\t%.2f\t\t%.2f\n", month, balance, interest, payment);
    		month++;
    		total_interest += interest;
    		}
    	if(silent != 0)
    		{
    		printf("Term = %d months.\n", (month -1));
    		printf("Total interest = %.2f\n", total_interest);
    		}
    	
    	return((month - 1));
    	}
    float payment(float balance, float rate, int term)
    	{
    	float pvif;
    	int i;
    	pvif = (1 + rate);
    	for(i = 0; i < term; i++)
    		{
    		pvif = pvif * (1 + rate);
    		}
    	return((balance * pvif) / ((pvif - 1) / rate));
    	}
    int main()
    {
    
    float balance, rate, minpay, payment_fixed;
    int term, selection, silent;
    float guess, value, guess_p;
    // Global variables:
    
    printf("Choose payment type:\n");
    
    printf("\t1: Minimum payment (credit card)\n");
    printf("\t2: Fixed payment (variable term)\n");
    printf("\t3: Fixed Term\n");
    printf("\t4: Calculate APR from fixed payment and term\n");
    printf("Selection: ");
    
    scanf("%d", &selection);
    switch (selection) {
    	case 1 :
    		// get balance, rate, minimum payment amount.
    		balance =  getbalance();
    		rate = getrate();
    		// Get minimum payment amount in percent
    		printf("Enter the minimum payment amount in percent.\n");
    		scanf("%f", &minpay);
    
    		iterate(balance, rate, minpay, 0, 1);
    		break;
    	case 2 :
    		// get balance, rate, fixed payment amount.
    		balance = getbalance();
    		rate = getrate();
    		payment_fixed = getpayment();
    		iterate(balance, rate, 0, payment_fixed, 1);
    		break;
    	case 3 :
    		//get balance, rate, term.
    		balance = getbalance();
    		rate = getrate();
    		term = getterm();
    		printf("Payment = %.2f\n", payment(balance, rate, term));
    		break;
    	case 4 :
    		// get balance, term, payment.
    		balance = getbalance();
    		term = getterm();
    		payment_fixed = getpayment();	
    		silent = 0;
    		// starting guess
    		guess = 10.0;
    		value = 1;	
    		while (value > 0.001)
    			{
    			if(iterate(balance, (guess / 1200), 0, payment_fixed, 0) < term)
    				{
    				guess_p = guess;
    				guess = guess + value;
    				}
    			else
    				{
    				guess = guess_p;
    				value = value / 10;
    				}
    			}
    		printf("APR = %.2f\%\n", guess);
    		break;
    	default :
    		printf("Bad input, Quitting!\n");
    		break;
    	}
    return 0;
    }
    Its not totally accurate and i'm still improving it. Just looking for any insight on the way i'm doing things. Sorry about the lack of comments.
    Thanks.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Code:
    			if(iterate(balance, (guess / 1200), 0, payment_fixed, 0) < term)
    				{
    				guess_p = guess;
    				guess = guess + value;
    				}
    			else
    				{
    				guess = guess_p;
    				value = value / 10;
    				}
    			}
    Would a "midpoint search" be a better option here? So instead of dividing by 10 each time, have a guess_low and guess_high, guess = (guess_low + guess_high) / 2, then perform set guess_low or guess_high to guess, depending on if it was low or high. Exit would be based on guess_low - guess_high being "small enough".

    Code:
    		printf("Bad input, Quitting!\n");
    		break;
    I prefer programs that let you go back to the beginning instead of quitting if you give bad input.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Ok, just as i thought, i was being a bit daft with my function calls. Instead of doing:

    Code:
    float getbalance(void)
    	{
            float balance;
    	printf("Enter balance.\n");
    	scanf("%f", &balance);
    	return(balance);
    	}
    and then in main() doing:
    Code:
    	
    float balance;
    balance =  getbalance();
    I can clearly just do:
    Code:
    float getbalance(balance)
    	{
    	printf("Enter balance.\n");
    	scanf("%f", &balance);
    	return(balance);
    	}
    
    int main()
    {
    float balance;
    balance = getbalance(balance);
    No?

  6. #6
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Quote Originally Posted by matsp View Post
    Code:
    			if(iterate(balance, (guess / 1200), 0, payment_fixed, 0) < term)
    				{
    				guess_p = guess;
    				guess = guess + value;
    				}
    			else
    				{
    				guess = guess_p;
    				value = value / 10;
    				}
    			}
    Would a "midpoint search" be a better option here? So instead of dividing by 10 each time, have a guess_low and guess_high, guess = (guess_low + guess_high) / 2, then perform set guess_low or guess_high to guess, depending on if it was low or high. Exit would be based on guess_low - guess_high being "small enough".

    Code:
    		printf("Bad input, Quitting!\n");
    		break;
    I prefer programs that let you go back to the beginning instead of quitting if you give bad input.

    --
    Mats
    2 really good ideas! Thanks!

    I don't know how i would make the program go back to the beginning though?
    Put the whole thing in a while loop? But would each case in the switch have to set a flag or something? I really am new to this!!
    Going to have a look at your midpoint suggestion. Thanks.

  7. #7
    Woof, woof! zacs7's Avatar
    Join Date
    Mar 2007
    Location
    Australia
    Posts
    3,459
    Yes, or you could just call the a function again (NOT main()!)

    For example:
    Code:
    #include <stdio.h>
    
    int main(void)
    {
       return my_program();
    }
    
    /* returns non-zero on error */
    int prompt(float * var, const char * message)
    {
       char line[BUFSIZ];   /* Garanteed to be at least 256 bytes */
    
       if(message == NULL || var == NULL)
          return 1;      /* error */
    
       printf("&#37;s\n", message);
    
       /* use fgets() to prevent input stream issues (could add more error checking) */
       fgets(line, sizeof(line), stdin);
       sscanf(line, "%f", var);
       return 0;
    }
    
    /* Passes return value to main() */
    int my_program()
    {
       int display = 1,
           selection = 0;
    
       float balance = 0.0f,
             term = 0.0f,
             rate = 0.0f;
    
       while(display)
       {
          /* print menu */
          printf(  "Choose payment type:\n"
                   "\t1. Minimum payment (credit card)\n"
                   "\t2. Fixed payment (variable term)\n"
                   "\t3. Fixed term\n"
                   "\t4. Calculate APR from fixed payment and term\n"
                   "\t5. Quit\n");
    
          /* you could adapt prompt here (or make it more generic) */
    
    
          /* Each "option" requires, 1 a balance, 2 a rate, 3 a term
           * remove these from each switch case, ask only if the option was valid */
          if(selection >= 1 && selection <= 4)
          {
             /* TODO: error checking */
             prompt(&rate, "Enter the interest (APR) in percent.");
             prompt(&balance, "Enter balance.");
             /* TODO: get term as months :) */
          }
    
          switch(selection)
          {
             /* minimum */
             case 1:
                /* TODO: error checking */
                prompt(&minpay, "Minimum payment amount in percent.");
                iterate(balance, rate, minpay, 0, 1);
             break;
    
             /* ... */
    
             /* quit */
             case 5:
                display = 0;        /* quit loop */ 
             break;
    
             default:
                printf("Unknown option");
          }
       }
       return 0;
    }
    Note: I didn't actually do that. But I left the "template" there so you could see how it could be done.

    I did make a few suggestions...

  8. #8
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Thanks for the reply!!
    Its things like that which make me realise how new i am to this!

    I've done something similar to your idea now, with a while loop in main.

    So my code is now:
    Code:
    #include<stdio.h>
    
    float getbalance(float balance)
    	{
    	printf("Enter balance.\n");
    	scanf("%f", &balance);
    	return(balance);
    	}
    
    float getrate(float rate)
    	{
    	printf("Enter the interest (APR) in percent.\n");
    	scanf("%f", &rate);
    	rate = rate / 1200;
    	return(rate);
    	}
    int getterm(int term)
    	{
    	printf("Enter the term in months.\n");
    	scanf("%d", &term);
    	return(term);
    	}
    float getpayment(float payment_fixed)
    	{
    	printf("Enter the payment amount ().\n");
    	scanf("%f", &payment_fixed);
    	return(payment_fixed);
    	}
    
    int iterate(float balance, float rate, float minpay, float payment_fixed, int silent)
    	{
    	float interest, payment;
    	float total_interest = 0;
    	int month = 1;
    	while (balance > 0)
    		{
    		interest = rate * balance;
    			if(interest <= 0.5)
    				if(rate == 0)
    					interest = 0;
    				else
    					interest = 0.5;
    		payment = ((minpay / 100) * balance) + payment_fixed;
    		balance = balance + interest;	
    			if(payment <= 5)
    				payment = 5;
    			if(balance <= payment)
    				payment = balance;
    		balance = balance - payment;	
    			while(silent)
    				printf("%d\t%.2f\t\t%.2f\t\t%.2f\n", month, balance, interest, payment);
    		month++;
    		total_interest += interest;
    		}
    	while(silent)
    		{
    		printf("Term = %d months.\n", (month -1));
    		printf("Total interest = %.2f\n", total_interest);
    		}
    	
    	return((month - 1));
    	}
    float payment(float balance, float rate, int term)
    	{
    	float pvif;
    	int i;
    	pvif = (1 + rate);
    	for(i = 0; i < term; i++)
    		{
    		pvif = pvif * (1 + rate);
    		}
    	return((balance * pvif) / ((pvif - 1) / rate));
    	}
    int main()
    {
    
    float balance, rate, minpay, payment_fixed;
    int term, selection, silent;
    float guess, value, guess_p;
    // Global variables:
    int display = 1;
    while(display)
    {
    printf("Choose payment type:\n");
    
    printf("\t1: Minimum payment (credit card)\n");
    printf("\t2: Fixed payment (variable term)\n");
    printf("\t3: Fixed Term\n");
    printf("\t4: Calculate APR from fixed payment and term\n");
    printf("\t5: Quit\n");
    printf("Selection: ");
    
    scanf("%d", &selection);
    switch (selection) {
    	case 1 :
    		// get balance, rate, minimum payment amount.
    		balance =  getbalance(balance);
    		rate = getrate(rate);
    		// Get minimum payment amount in percent
    		printf("Enter the minimum payment amount in percent.\n");
    		scanf("%f", &minpay);
    
    		iterate(balance, rate, minpay, 0, 1);
    		break;
    	case 2 :
    		// get balance, rate, fixed payment amount.
    		balance = getbalance(balance);
    		rate = getrate(rate);
    		payment_fixed = getpayment(payment_fixed);
    		iterate(balance, rate, 0, payment_fixed, 1);
    		break;
    	case 3 :
    		//get balance, rate, term.
    		balance = getbalance(balance);
    		rate = getrate(rate);
    		term = getterm(term);
    		printf("Payment = %.2f\n", payment(balance, rate, term));
    		break;
    	case 4 :
    		// get balance, term, payment.
    		balance = getbalance(balance);
    		term = getterm(term);
    		payment_fixed = getpayment(payment_fixed);	
    		silent = 0;
    		// starting guess
    		guess = 10.0;
    		value = 1;	
    		while (value > 0.001)
    			{
    			if(iterate(balance, (guess / 1200), 0, payment_fixed, 0) < term)
    				{
    				guess_p = guess;
    				guess = guess + value;
    				}
    			else
    				{
    				guess = guess_p;
    				value = value / 10;
    				}
    			}
    		printf("APR = %.2f\%\n", guess);
    		break;
    	case 5 :
    		printf("Quitting.\n");
    		display = 0;
    		break;
    	default :
    		printf("Bad input.\n");
    		break;
    	}
    }
    return 0;
    }
    Better?

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,897
    All those get_XXX functions take an argument which is quite useless.
    You can actually define those variables local and have it return it instead.
    Example:
    Code:
    int getterm()
    {
    	int term;
    	printf("Enter the term in months.\n");
    	scanf("&#37;d", &term);
    	return term;
    }
    Indentation is also messed up in several places.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    I had something similar originally. But i thought i'd need a global variable otherwise it would keep asking for input whenever the function was called. So i defined a global variable and used that instead, getting rid of the local definitions in the functions.

    Also, i thought my indentation was pretty good!
    Except the while(display) loop!

    Thanks.

  11. #11
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,897
    Quote Originally Posted by AmbliKai View Post
    I had something similar originally. But i thought i'd need a global variable otherwise it would keep asking for input whenever the function was called. So i defined a global variable and used that instead, getting rid of the local definitions in the functions.
    Uh? The get_XXX functions is supposed to ask the user for input. If it does not, then it isn't a get function...
    Also, global variables are error-prone and should be avoided.

    Also, i thought my indentation was pretty good!
    I see you've taken the liberty to indent a little more at some places?
    It's not really necessary though.
    And yes, your indentation in the while loop in main is messed up. Or rather, main itself has its indentation messed up.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  12. #12
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Quote Originally Posted by Elysia View Post
    Uh? The get_XXX functions is supposed to ask the user for input. If it does not, then it isn't a get function...
    Also, global variables are error-prone and should be avoided.


    I see you've taken the liberty to indent a little more at some places?
    It's not really necessary though.
    And yes, your indentation in the while loop in main is messed up. Or rather, main itself has its indentation messed up.
    Yeah, i'm kinda new to this and self taught, i tried to indent it but i'm probably not really getting the idea! Could you suggest how to do it properly? (i should really have a search on the net!)If i knew an actual 'method' as such i could try to stick to it. That would be cool.

    With the get_xxx functions, i thought if i had a local variable it would say ask for the balance more than once in each switch case?

    On closer inspection i can see that wouldn't be the case though (i think)!
    So i should have: for example case 1:
    Code:
    		printf("Enter the minimum payment amount in percent.\n");
    		scanf("%f", &minpay);
    		iterate(getbalance(), getrate(), minpay, 0, 1);
    		break;
    Is this what you mean? Looks a lot more concise to me.

    Also, apologies for the 20 questions but whats wrong with global varibles? My book doesn't say anything (in the little i've covered) about that. Is it just to do with memory overwriting pre allocated space or similar?

    Thanks a lot for your time Elysia. Much appreciated!

  13. #13
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    22,897
    Quote Originally Posted by AmbliKai View Post
    Yeah, i'm kinda new to this and self taught, i tried to indent it but i'm probably not really getting the idea! Could you suggest how to do it properly? (i should really have a search on the net!)If i knew an actual 'method' as such i could try to stick to it. That would be cool.
    Here's an old guide I wrote a long time ago: http://cpwiki.sourceforge.net/User:Elysia/Indentation

    With the get_xxx functions, i thought if i had a local variable it would say ask for the balance more than once in each switch case?

    On closer inspection i can see that wouldn't be the case though (i think)!
    So i should have: for example case 1:
    Code:
    		printf("Enter the minimum payment amount in percent.\n");
    		scanf("%f", &minpay);
    		iterate(getbalance(), getrate(), minpay, 0, 1);
    		break;
    Is this what you mean? Looks a lot more concise to me.
    That looks better to me.
    Currently, the functions just use the parameter to store the temporary answer and return it, so it could just as well be a local variable and function correctly.

    Also, apologies for the 20 questions but whats wrong with global varibles? My book doesn't say anything (in the little i've covered) about that. Is it just to do with memory overwriting pre allocated space or similar?
    Global variables aren't wrong per se, but since they are global, it means any function can change them anywhere and anytime, so it causes bugs.
    Global variables are a potent tool, but they should be used carefully.
    Also, some compilers will have trouble optimizing global variables because it can't say when or where they're used. That probably doesn't mean much to you now, but if at any point in the future, you intend to write programs, it's a good thing to keep in mind.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  14. #14
    Registered User
    Join Date
    Oct 2007
    Location
    Glasvegas, Scotland.
    Posts
    68
    Thanks for the guide!

    I've just found why i can't use getbalance() as a local variable. At least at one point.

    the code:
    Code:
    			
    			if(iterate(balance, (guess / 1200), 0, payment_fixed, 0) < term)
    				{
    				guess_p = guess;
    				guess = guess + value;
    				}
    			else
    				{
    				guess = guess_p;
    				value = value / 10;
    				}
    			}
    In case 4 is within a while loop and will ask for the balance repeatedly untill the loop has finished. Should i use a global variable in this case only and use a local variable within the function in the other cases?

    Also, will a variable declared inside the switch be local? So i could initialise float balance at the beginning of the case switch? Then it wouldn't be global?

    Cheers.

  15. #15
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    You can certainly use a local variable in getBalance, as long as you return a value into a main local varaible (that is NOT the same as a global variable). Passing "balance" into getBalance doesn't serve any purpose.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Values changing without reason?
    By subtled in forum C Programming
    Replies: 2
    Last Post: 04-19-2007, 11:20 AM
  2. Obfuscated Code Contest: The Results
    By Stack Overflow in forum Contests Board
    Replies: 29
    Last Post: 02-18-2005, 05:39 PM
  3. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 04:17 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 06:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21