ansaurus

Question

Having trouble debugging variable might not have been initialized error

Answer 1

+4 A:

What happens if typeOfFrame is not R, r, F, or f? A possible solution is to add a "default" case to the switch.

I think this code is closer to what you want (I reduced a lot of duplication). Note that I did not really deal with errors (the calls to isValidChoice and isFrameType methods are a start with how to deal with them). The issue above was that you are basically assuming that the user will only ever enter in valid input and forgetting that there are times that, even though, Y and N are the only valid choices they could enter in Q. I also fixed a bug where you were not always printing out the grand total, and were not setting the grand total (the last part above the printf).

The last suggestion I have is do NOT use doubles for this (unless you were told to by your instructor) because they will give you inaccurate numbers in some cases. Take a look at java.math.BigDecimal instead.

class Main
{
    static Scanner console = new Scanner(System.in);    
    static final double REGULAR_FRAME = .15;
    static final double FANCY_FRAME = .25;
    static final double COLOR = .10;
    static final double CARDBOARD = .02;
    static final double GLASS = .07;
    static final double CROWNS = .35;

    public static void main (String[] args)
    {   
        final double length;
        final double width;
        final char   typeOfFrame;
        final char   choiceOfColor;

        System.out.println ("Please enter the length of your picure in inches:");
        length = console.nextDouble();

        System.out.println ("Please enter the width of your picure in inches: ");
        width = console.nextDouble();

        System.out.println ("Please enter the type of frame: R or r (Regular), F or f (Fancy). ");  
        typeOfFrame = console.next().charAt(0);

        System.out.println ("Would you like to add color?: Y for (Yes), N for (No): "); 
        choiceOfColor = console.next().charAt(0);

        if(!(isFrameType(typeOfFrame)))
        {

        }
        else
        {
            final double area;
            final double perimeter; 
            final double priceOfFrame;
            final double priceOfCardboard;
            final double priceOfGlass;

            area             = (length * width);
            perimeter        = (2 * length) + (2 * width);
            priceOfFrame     = (perimeter * REGULAR_FRAME);
            priceOfCardboard = (area * CARDBOARD);
            priceOfGlass     = (area * GLASS);  

            if(isValidChoice(choiceOfColor))
            {
                final double priceOfColor;
                final double finalPrice;
                final char   choiceOfCrowns;
                final double grandTotalPrice; 

                if(choiceOfColor == 'N')
                {
                    finalPrice = (priceOfFrame + priceOfCardboard + priceOfGlass);
                }
                else
                {
                    priceOfColor = (area * COLOR);          
                    finalPrice   = (priceOfFrame + priceOfColor + priceOfCardboard + priceOfGlass);     
                }    

                System.out.println ("Would you like to add crowns? Enter Y (Yes), or N (No): ");    
                choiceOfCrowns = console.next().charAt(0);

                if(isValidChoice(choiceOfCrowns))
                {
                    if(choiceOfCrowns == 'Y')
                    {
                        final double crownFinalPrice;
                        final int    numberOfCrowns;

                        System.out.println ("How many crowns would you like? ");    
                        numberOfCrowns  = console.nextInt();        
                        crownFinalPrice =(numberOfCrowns * CROWNS);
                        grandTotalPrice = (crownFinalPrice + finalPrice);
                    }   
                    else
                    {
                        grandTotalPrice = finalPrice;
                    }

                    System.out.printf ("Your total comes to: $%.2f%n", grandTotalPrice);    
                }
            }
        }
    }   

    private static boolean isFrameType(final char c)
    {        
        final char lower;

        lower = Character.toLowerCase(c);

        return (lower == 'r' || lower == 'f');
    }

    private static boolean isValidChoice(final char c)
    {
        return (c == 'Y' || c == 'N');
    }
}

TofuBeer 2010-10-23 00:11:23

+1, both gets to the root of the questioner's problem and redirects towards the bigger picture.

andersoj 2010-10-23 01:04:49

Thank you TofuBeer! This is very helpful and informative.

barnaby jones 2010-10-23 14:03:14

Answer 2

A:

This is happening because the compiler is warning you that the values might never have been set during some runs of the application. They are set in the switch and if statements, but the compiler warns you that some input may lead to them not being initialized.

Simply setting them to a value when you define them would be enough:

double finalPrice = 0, grandTotalPrice = 0;

but you could also ensure that they are set no matter what path the application takes (knowning all possible paths is good practice anyway).

steinar 2010-10-23 00:12:32

Yes, but this is only sidestepping/neutralizing a very useful compiler warning, rather than exploiting it to ensure correctness. Unless you are explicitly and meaningfully initializing a mutable variable (say, an accumulator) you shouldn't initialize them just to make the code compile -- instead make sure the variable is set correctly in all cases.

andersoj 2010-10-23 00:15:19

That's why I noted that "knowing all possible paths was a good practice anyway". Maybe I wasn't clear enough. If he's sure that he has covered the paths of the code, initializing the values takes care of those paths where he's not setting the values explicitly. It's a pattern I guess most of us more experienced programmers follow every day. No reason to not mentioning that.

steinar 2010-10-23 00:19:02

@steinar: I missed it, I guess. Sorry. To me it was clear that the unhandled paths were straightforward and could be brought under control fairly easily. My knee-jerk reaction is to consider any variable initialization to 0.0 an anti-pattern, more often wrong/lazy than correct. (Obviously not exclusively...)

andersoj 2010-10-23 01:12:39

Answer 3

+3 A:

Here is a first-pass refactoring of your code, addressing your specific question as well as a few others. Some observations that were addressed in the refactoring:

In response to your question (and some of the other answers) don't get in the habit of blindly initializing variables just to make the compiler errors go away. The compiler error here is an indication that you need to clearly define the variable (meaningfully for your application) regardless of the potential states. Initializing the variable just hides the problem that you have uncontrolled/unexpected states that haven't been dealt with.
The main() method is too long and has too many things in it. The refactoring I show is a good example of a "next step," by no means a completed process... but the first thing was to pull out some of the logic into a digestible subset. Software construction relies on breaking things into components you can reason about, and why not start with your frame price computer?
Related to the previous point, you need to work to scope variables down to the absolute narrowest possible scope. Locality is your friend. Minimize variable scope. This is aided by breaking things into separate methods, but it also applies to bringing variables into blocks, etc. Here's a good related SO discussion. Also, Effective Java Item 45: Minimize the scope of local variables.
This one is controversial, but I'll offer my opinion. Use final everywhere you can. Mutability is not your friend, in most cases, and this code is a great example -- you've got bits and pieces taking on values in a lot of different places, when in fact most of the values can be defined once and categorically, and the other computations can proceed linearly. Its more readable/maintainable, and you'll be amazed at the number of logical errors you'll find at the compile stage rather than in iterative debugging by doing this. Favor immutability, allow mutability judiciously and only with cause. See my answer on this question for some discussion, and several related, linked SO questions. Also, Effective Java Item 15: Minimize Mutability.
Prefer enum to String or char or whatever. Especially when coping with potentially misbehaving input from a user or external system, your overriding goal should be to reduce the state space to the absolute minimum, and get things into a controlled vocabulary. I did a first pass here, but your code should throw exceptions or emit error messages to the user (depending on the level of abstraction; here emitting errors and asking again for input) as close to the interface as is possible, not allowing unstructured/dirty data to propagate deeper into the application than is necessary. The deeper it goes, the less context you have, and the uglier/messier the logic to cope with a failure becomes. Then, you accrete error handling logic that makes dead-simple internal computations untestable.
Duplication is a code smell. If you're computing the area in two different places, refactor it to avoid. Related on SO: "What duplication threshold...?", but the duplication threshold for something as simple as area = length x width is EXACTLY once. Minimize or eliminate duplicate code.
Use Exception for exceptional conditions. This actually closes the loop with my first point and your question. Your code had variables which the compiler wasn't sure were being initialized because there was no well-defined value for them to be initialized to in the exceptional cases -- they represented erroneous input, not a compiler warning to squash. Get the state space under control, and when an unintended state is encountered, THROW AN EXCEPTION. Better an explicit and interpretable, potentially recoverable error than a silent but incorrect result.

There's more, but that's a useful set of critiques, I think, and more than you asked for. The code:

import java.util.*;

public final class PictureFrames
{

  static Scanner console = new Scanner(System.in);

  static final double REGULAR_FRAME = .15, FANCY_FRAME = .25;
  static final double COLOR = .10, CARDBOARD = .02, GLASS = .07, CROWNS = .35;

  enum FrameType {
    /** Regular. */
    R, 
    /** Fancy. */
    F;
  };


  static double areaPriceInDollars(final FrameType frameType,
                                   final double length,
                                   final double width,
                                   final boolean color)
  {
    final double area,perimeter,
      priceOfFrame,
      priceOfCardboard,
      priceOfGlass,
      priceOfColor;

      area = length * width;
      perimeter = 2 * (length + width);

      priceOfCardboard = (area * CARDBOARD);
      priceOfGlass = (area * GLASS);      

      if (color) 
        priceOfColor = area * COLOR;
      else 
        priceOfColor = 0.0;

      switch (frameType) {
        case R:
          priceOfFrame = (perimeter * REGULAR_FRAME);
          break;
        case F:
          priceOfFrame = (perimeter * FANCY_FRAME);
          break;
        default:
          throw new IllegalArgumentException("FrameType "+frameType+" unknown, no price available.");
      }

      return priceOfColor + priceOfCardboard + priceOfGlass + priceOfFrame;
    }       


  public static void main(String[] args)
  {
    System.out.println("Please enter the length of your picure in inches:");
    final double length = console.nextDouble();

    System.out.println("Please enter the width of your picure in inches: ");
    final double width = console.nextDouble();

    System.out
        .println("Please enter the type of frame: R or r (Regular), F or f (Fancy). ");
    final char typeOfFrame = console.next().charAt(0);
    FrameType frameType = FrameType.valueOf(""
        + Character.toUpperCase(typeOfFrame));

    System.out
        .println("Would you like to add color?: Y for (Yes), N for (No): ");
    final char choiceOfColor = console.next().charAt(0);
    final boolean color = Character.toUpperCase(choiceOfColor) == 'Y';

    System.out
        .println("Would you like to add crowns? Enter Y (Yes), or N (No): ");
    final char choiceOfCrowns = console.next().charAt(0);
    final boolean crowns = Character.toUpperCase(choiceOfCrowns) == 'Y';

    final double priceOfCrowns;
    if (crowns) {
      System.out.println("How many crowns would you like? ");
      final int numberOfCrowns = console.nextInt();
      priceOfCrowns = (numberOfCrowns * CROWNS);
    } else {
      priceOfCrowns = 0.0;
    }

    final double grandTotalPrice = priceOfCrowns
        + areaPriceInDollars(frameType, length, width, color);
    System.out.printf("Your total comes to: $%.2f%n", grandTotalPrice);
  }
}

andersoj 2010-10-23 00:53:48

Thank you andersoj! This is incredibly helpful and informative. Thank you for all of the fantastic information and the links. I really appreciate the help and the additional information.

barnaby jones 2010-10-23 13:55:11

@Karl: My pleasure!

andersoj 2010-10-23 15:36:49

@Karl: (And accept the answer, if you like it. /selfish)

andersoj 2010-10-23 15:37:11

ansaurus

tags:

views:

answers:

Having trouble debugging variable might not have been initialized error

related questions