Three suggestions before I delve any deeper into the code:
- Declare variables as late as you can to make it easier to understand the code.
- Refactor this giant method – it’s unreadably huge at the moment.
- Make the constants
static finalfields. They’re not related to any particular call to the method, so they shouldn’t be local variables.
Now as to the actual question, the simplest way is to make sure that every possible flow actually does assign a value or throw an exception. So for textCost, change your code to:
if (numberOfWords <= 25)
{
textCost = TEXT_BASE_FEE + (TEXT_FLAT_FEE * numberOfWords);
}
else if (numberOfWords <= 35)
{
textCost = TEXT_BASE_FEE + (TEXT_FLAT_FEE * 25) + (numberOfWords - 25) *
LESS_OR_EQUAL_THAN_THIRTYFIVE;
}
else // Note - no condition.
{
textCost = numberOfWords * MORE_THAN_THIRTYFIVE;
}
For linkCost, change your switch statement to something like:
switch (advPay)
{
case 'y':
linkCost = (3 * LINK_FLAT_FEE) -
(3 * LINK_FLAT_FEE) * THREE_MONTH_ADV_DISCOUNT;
break;
case 'n':
linkCost = LINK_FLAT_FEE;
break;
default:
throw new Exception("Invalid value specified: " + advPay);
}
Now you may not want to throw an exception here. You might want to loop round again, or something like that. You probably don’t want to use just bare Exception – but you should think about the exact exception type you do want to use.
It’s not always possible to do this. The rules by the compiler to determine definite assignment are relatively straightforward. In cases where you really can’t change the code to make the compiler happy like this, you can just assign a dummy initial value. I’d recommend trying to avoid this wherever possible though. In your first case, the value really would always be assigned – but in the second case you really weren’t giving a value when advPay was neither ‘y’ nor ‘n’ which could lead to a hard-to-diagnose problem later on. The compiler error helps you spot this sort of problem.
Again though, I strongly suggest you refactor this method. I suspect you’ll find it a lot easier to understand why things aren’t definitely assigned when there’s only about 10 lines of code to reason about in each method, and when each variable is declared just before or at its first use.
EDIT:
Okay, the radically refactored code is below. I’m not going to claim it’s the best code in the world, but:
- It’s more testable. You could easily write unit tests for each part of it.
printAllCostsisn’t terribly easily testable, but you could have an overload which took aWriterto print to – that would help. - Each bit of calculation is in a logical place. Links and graphics have a small set of possible values – Java enums are a natural fit here. (I’m aware they may well be beyond your current skill level, but it’s good to see what will be available.)
- I’m not using binary floating point numbers any more, because they’re inappropriate for numbers. Instead, I’m using an integer number of cents everywhere and converting to
BigDecimalfor display purposes. See my article on .NET floating point for more information – it’s all relevant to Java really. - The advert itself is now encapsulated in a class. You could add a lot more information here as and when you needed to.
- The code is in a package. Admittedly it’s all in one file at the moment (which is why only the
EntryPointclass is public) but that’s just for the sake of Stack Overflow and me not having to open up Eclipse. - There’s JavaDoc explaining what’s going on – at least for a few methods. (I would probably add some more in real code. I’m running out of time here.)
- We validate user input, except for the word count – and we perform that validation in a single routine, which should be reasonably testable. We can then assume that whenever we’ve asked for input, we’ve got something valid.
- The number of static methods in
EntryPointis slightly alarming. It doesn’t feel terribly OO – but I find that’s often the way around the entry point to a program. Note that there’s nothing to do with fees in there – it’s just the user interface, basically.
There’s more code here than there was before – but it’s (IMO) much more readable and maintainable code.
package advertising;
import java.util.Scanner;
import java.math.BigDecimal;
/** The graphic style of an advert. */
enum Graphic
{
NONE(0),
SMALL(1907),
MEDIUM(2476),
LARGE(2933);
private final int cost;
private Graphic(int cost)
{
this.cost = cost;
}
/** Returns the cost in cents. */
public int getCost()
{
return cost;
}
}
/** The link payment plan for an advert. */
enum LinkPlan
{
NONE(0),
PREPAID(1495), // 1 month
POSTPAID(1495 * 3 - (1495 * 3) / 10); // 10% discount for 3 months up-front
private final int cost;
private LinkPlan(int cost)
{
this.cost = cost;
}
/** Returns the cost in cents. */
public int getCost()
{
return cost;
}
}
class Advertisement
{
private final int wordCount;
private final LinkPlan linkPlan;
private final Graphic graphic;
public Advertisement(int wordCount, LinkPlan linkPlan, Graphic graphic)
{
this.wordCount = wordCount;
this.linkPlan = linkPlan;
this.graphic = graphic;
}
/**
* Returns the fee for the words in the advert, in cents.
*
* For up to 25 words, there's a flat fee of 40c per word and a base fee
* of $3.00.
*
* For 26-35 words inclusive, the fee for the first 25 words is as before,
* but the per-word fee goes down to 35c for words 26-35.
*
* For more than 35 words, there's a flat fee of 32c per word, and no
* base fee.
*/
public int getWordCost()
{
if (wordCount > 35)
{
return 32 * wordCount;
}
// Apply flat fee always, then up to 25 words at 40 cents,
// then the rest at 35 cents.
return 300 + Math.min(wordCount, 25) * 40
+ Math.min(wordCount - 25, 0) * 35;
}
/**
* Displays the costs associated with this advert.
*/
public void printAllCosts()
{
System.out.printf("\t\t%-16s %11s\n", "Category", "Cost");
printCost("Text", getWordCost());
printCost("Link", linkPlan.getCost());
printCost("Graphic", graphic.getCost());
int total = getWordCost() + linkPlan.getCost() + graphic.getCost();
printCost("Total", total);
int gst = total / 20;
printCost("GST", gst);
printCost("Total with GST", total + gst);
}
private void printCost(String category, int cents)
{
BigDecimal dollars = new BigDecimal(cents).scaleByPowerOfTen(-2);
System.out.printf("\t\t%-16s %11.2f\n", category, dollars);
}
}
/**
* The entry point for the program - takes user input, builds an
* Advertisement, and displays its cost.
*/
public class EntryPoint
{
public static void main(String[] args)
{
Scanner scanner = new Scanner(System.in);
System.out.println("Welcome!");
int wordCount = readWordCount(scanner);
LinkPlan linkPlan = readLinkPlan(scanner);
Graphic graphic = readGraphic(scanner);
Advertisement advert = new Advertisement(wordCount, linkPlan, graphic);
advert.printAllCosts();
}
private static int readWordCount(Scanner scanner)
{
System.out.print("Enter the number of words in your ad: ");
// Could add validation code in here
return scanner.nextInt();
}
private static LinkPlan readLinkPlan(Scanner scanner)
{
System.out.print("Would you like to add a link (y = yes or n = no)? ");
char addLink = readSingleCharacter(scanner, "yn");
LinkPlan linkPlan;
if (addLink == 'n')
{
return LinkPlan.NONE;
}
System.out.print("Would you like to pay 3 months in advance " +
"(y = yes or n = no)? ");
char advancePay = readSingleCharacter(scanner, "yn");
return advancePay == 'y' ? LinkPlan.PREPAID : LinkPlan.POSTPAID;
}
private static Graphic readGraphic(Scanner scanner)
{
System.out.print("Would you like to add graphics/pictures? " +
"(s = small, m = medium, l = large or n = None)? ");
char graphic = readSingleCharacter(scanner, "smln");
switch (graphic)
{
case 's': return Graphic.SMALL;
case 'm': return Graphic.MEDIUM;
case 'l': return Graphic.LARGE;
case 'n': return Graphic.NONE;
default:
throw new IllegalStateException("Unexpected state; graphic=" +
graphic);
}
}
private static char readSingleCharacter(Scanner scanner,
String validOptions)
{
while(true)
{
String input = scanner.next();
if (input.length() != 1 || !validOptions.contains(input))
{
System.out.print("Invalid value. Please try again: ");
continue;
}
return input.charAt(0);
}
}
}