StackOverflow2013

Note that there are some explanatory texts on larger screens.

plurals

PO
text
Body
copied!<p>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:</p> <ul> <li><p>In response to your question (and some of the other answers) <strong>don't get in the habit of blindly initializing variables just to make the compiler errors go away</strong>. 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.</p></li> <li><p>The <code>main()</code> <strong>method is too long</strong> 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?</p></li> <li><p>Related to the previous point, you need to work to scope variables down to the absolute narrowest possible scope. Locality is your friend. <strong>Minimize variable scope.</strong> This is aided by breaking things into separate methods, but it also applies to bringing variables into blocks, etc. Here's a <a href="https://stackoverflow.com/questions/205458/named-blocks-to-limit-variable-scope-good-idea">good related SO discussion</a>. Also, <a href="http://java.sun.com/docs/books/effective/toc.html" rel="nofollow noreferrer"><em>Effective Java</em></a> Item 45: Minimize the scope of local variables.</p></li> <li><p>This one is controversial, but I'll offer my opinion. Use <code>final</code> 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 <em>compile</em> stage rather than in iterative debugging by doing this. <strong>Favor immutability, allow mutability judiciously and only with cause.</strong> See <a href="https://stackoverflow.com/questions/3972510/the-parameter-foo-should-not-be-assigned-whats-the-harm/3974612#3974612">my answer on this question</a> for some discussion, and several related, linked SO questions. Also, <a href="http://java.sun.com/docs/books/effective/toc.html" rel="nofollow noreferrer"><em>Effective Java</em></a> Item 15: Minimize Mutability.</p></li> <li><p><strong>Prefer <code>enum</code> to <code>String</code></strong> or <code>char</code> 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. </p></li> <li><p><strong>Duplication is a code smell.</strong> If you're computing the area in two different places, refactor it to avoid. Related on SO: <a href="https://stackoverflow.com/questions/759657/what-duplication-detection-threshold-do-you-use">"What duplication threshold...?"</a>, but the duplication threshold for something as simple as area = length x width is EXACTLY once. <strong>Minimize or eliminate duplicate code.</strong></p></li> <li><p><strong>Use <code>Exception</code> for exceptional conditions</strong>. 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.</p></li> </ul> <p>There's more, but that's a useful set of critiques, I think, and more than you asked for. The code:</p> <pre><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); } } </code></pre>

Querying!

Guidance

An individual column

Larger individual text columns get their own page to allow for proper reading.

SQuiL has stopped working due to an internal error.

If you are curious you may find further information in the browser console, which is accessible through the devtools (F12).

Reload