Help me optimise my code!

**AmbliKai** · 10-15-2008

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.

**Elysia** · 10-15-2008

Yeah, you can post it. No harm in doing that. If people are interested, you'll get some feedback.

**AmbliKai** · 10-15-2008

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.

**matsp** · 10-15-2008

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

**AmbliKai** · 10-15-2008

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?

**AmbliKai** · 10-15-2008

Originally Posted by matsp

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.

**zacs7** · 10-15-2008

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...

**AmbliKai** · 10-15-2008

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?

**Elysia** · 10-15-2008

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.

**AmbliKai** · 10-15-2008

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.

**Elysia** · 10-15-2008

Originally Posted by AmbliKai

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.

**AmbliKai** · 10-15-2008

Originally Posted by Elysia

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!

**Elysia** · 10-15-2008

Originally Posted by AmbliKai

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.

**AmbliKai** · 10-15-2008

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.

**matsp** · 10-15-2008

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

Thread: Help me optimise my code!

Thread Tools

Search Thread

Display

Help me optimise my code!

Similar Threads

Values changing without reason?

Obfuscated Code Contest: The Results

Obfuscated Code Contest

Interface Question

Who will map the scan code (inserted by VKD_Force_keys) to virtual key code?