Jak uniknąć „zmienna lokalna mogła nie zostać zainicjowana”?

/*This is a program that calculates Internet advertising rates based on what features/options you choose.
 * 
 *  
 */

import java.util.Scanner;

public class InternetAdvertising 
{
    public static void main(String[] args)
    {
        Scanner in = new Scanner(System.in);

        int numberOfWords;      

        //I assigned 0 values to both as Eclipse suggested
        float textCost = 0;
        float linkCost = 0;     

        float graphicCost;

        //<=25 words is a flat fee of $.40 per word plus Base fee of $3.00 
        final float TEXT_FLAT_FEE = 0.40F;
        final float TEXT_BASE_FEE = 3.00F;

        //<=35 words is $.40 for the first 25 words and 
        //an additional $.35 per word up to and including 35 words plus Base fee of $3.00 
        final float LESS_OR_EQUAL_THAN_THIRTYFIVE = 0.35F;

        //Over 35 words is a flat fee of $.32 per word with no base fee
        final float MORE_THAN_THIRTYFIVE = 0.32F;


        System.out.println("Welcome!");

        System.out.print("Enter the number of words in your ad: ");
        numberOfWords = in.nextInt();

        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 if (numberOfWords > 35)
        {
            textCost = numberOfWords * MORE_THAN_THIRTYFIVE;
        }


        String addLink, advancePay;
        char link, advPay;

        final float LINK_FLAT_FEE = 14.95F;
        final float THREE_MONTH_ADV_DISCOUNT = 0.10F;

        System.out.print("Would you like to add a link (y = yes or n = no)? ");
        addLink = in.next();

        link = addLink.charAt(0);
        link = Character.toLowerCase(link); 

        if (link == 'y')
        {
            System.out.print("Would you like to pay 3 months in advance " + "(y = yes or n = no)? ");
            advancePay = in.next();

            advPay = advancePay.charAt(0);
            advPay = Character.toLowerCase(advPay);

            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;
            }               
        }

        else
        {
            linkCost = 0;
        }


        String addGraphic;
        char graphic;

        System.out.print("Would you like to add graphics/pictures” + “(S = Small, M = Medium, L = Large or N = None)? ");
        addGraphic = in.next();

        graphic = addGraphic.charAt(0);
        graphic = Character.toUpperCase(graphic);
        graphic = Character.toLowerCase(graphic);       
        switch (graphic)
        {
            case 's':

                graphicCost = 19.07F;

                break;

            case 'm':

                graphicCost = 24.76F;

                break;

            case 'l':

                graphicCost = 29.33F;

                break;

            default:
                graphicCost = 0;
        }


        float gst, totalBeforeGst, totalAfterGst;

        final float GST_RATE = 0.05F;

        totalBeforeGst = textCost + linkCost + graphicCost; //textCost & linkCost would not initialize

        gst = totalBeforeGst * GST_RATE;

        totalAfterGst = totalBeforeGst + (totalBeforeGst * GST_RATE);


        System.out.printf("\t\t%-16s %11s\n", "Category", "Cost");
        System.out.printf("\t\t%-16s %11.2f\n", "Text", textCost);  //linkCost would not initialize
        System.out.printf("\t\t%-16s %11.2f\n", "Link", linkCost);  //textCost would not initialize 
        System.out.printf("\t\t%-16s %11.2f\n", "Graphic", graphicCost);
        System.out.printf("\t\t%-16s %11.2f\n", "Total", totalBeforeGst);
        System.out.printf("\t\t%-16s %11.2f\n", "GST", gst);
        System.out.printf("\t\t%-16s %11.2f\n", "Total with GST", totalAfterGst);
    }   
}

Już prawie skończyłem z tym kodem i Eclipse sugeruje przypisanie wartości 0 do textCost i linkCost. Czy jest jakiś inny sposób na obejście tego problemu. Jeśli nie przypiszę wartości 0, pojawi się błąd (zmienna lokalna XXX mogła nie zostać zainicjowana). Czy ktoś może mi wyjaśnić, dlaczego tak się dzieje, mimo że mam przypisane równania obu zmiennym?

Dzięki.

EDYCJA: Zrobiłem zgodnie z sugestią i zadeklarowałem zmienne tylko wtedy, gdy będę ich potrzebować. Dodałem także kilka komentarzy.


person HelloWorld    schedule 18.10.2009    source źródło
comment
Zaktualizowałem swoją odpowiedź, dodając wiele refaktoryzacji. Mam nadzieję że ci się spodoba.   -  person Jon Skeet    schedule 19.10.2009


Odpowiedzi (8)


Trzy sugestie, zanim zagłębię się w kod:

  • Deklaruj zmienne tak późno, jak to możliwe, aby ułatwić zrozumienie kodu.
  • Zmień tę gigantyczną metodę – w tej chwili jest ona nieczytelnie ogromna.
  • Utwórz pola stałych static final. Nie są one powiązane z żadnym konkretnym wywołaniem metody, więc nie powinny być zmiennymi lokalnymi.

A jeśli chodzi o właściwe pytanie, najprostszym sposobem jest upewnienie się, że każdy możliwy przepływ rzeczywiście przypisuje wartość lub zgłasza wyjątek. Zatem dla textCost zmień swój kod na:

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;
}

Dla linkCost zmień instrukcję switch na coś takiego:

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);
}

Teraz możesz nie chcieć zgłaszać tutaj wyjątku. Możesz chcieć jeszcze raz zapętlić się lub coś w tym rodzaju. Prawdopodobnie nie chcesz używać samego Exception - ale powinieneś pomyśleć o dokładnym typie wyjątku, którego chcesz użyć.

Nie zawsze jest to możliwe. Reguły kompilatora służące do określenia określonego przypisania są stosunkowo proste. W przypadkach, gdy naprawdę nie możesz zmienić kodu, aby kompilator był zadowolony, możesz po prostu przypisać fikcyjną wartość początkową. Zalecałbym jednak unikanie tego, jeśli to możliwe. W pierwszym przypadku wartość zawsze zostanie przypisana, ale w drugim przypadku tak naprawdę nie podałeś wartość, gdy advPay nie było ani „y”, ani „n”, co mogłoby prowadzić do trudnego - aby później zdiagnozować problem. Błąd kompilatora pomaga wykryć tego rodzaju problem.

Jednak ponownie zdecydowanie sugeruję refaktoryzację tej metody. Podejrzewam, że znacznie łatwiej będzie ci zrozumieć, dlaczego rzeczy nie są definitywnie przypisane, jeśli w każdej metodzie jest tylko około 10 wierszy kodu do uzasadnienia i gdy każda zmienna jest deklarowana tuż przed lub przy pierwszym użyciu.

EDYTOWAĆ:

OK, radykalnie przebudowany kod znajduje się poniżej. Nie twierdzę, że to najlepszy kod na świecie, ale:

  • Jest bardziej testowalny. Można łatwo napisać testy jednostkowe dla każdej jego części. printAllCosts nie jest zbyt łatwo testowalny, ale możesz mieć przeciążenie, którego wydrukowanie wymagałoby Writer - to by pomogło.
  • Każdy bit obliczeń znajduje się w logicznym miejscu. Linki i grafika mają niewielki zestaw możliwych wartości - wyliczenia Java są tutaj naturalnym rozwiązaniem. (Mam świadomość, że mogą one przekraczać Twój obecny poziom umiejętności, ale dobrze jest zobaczyć, co będzie dostępne.)
  • Nie używam już binarnych liczb zmiennoprzecinkowych, ponieważ są one nieodpowiednie dla liczb. Zamiast tego używam wszędzie całkowitej liczby centów i konwertuję na BigDecimal na potrzeby wyświetlania. Zobacz mój artykuł na temat zmiennoprzecinkowy .NET, aby uzyskać więcej informacji — wszystko to naprawdę dotyczy języka Java .
  • Sama reklama jest teraz zamknięta w klasie. Możesz dodać tutaj o wiele więcej informacji, kiedy zajdzie taka potrzeba.
  • Kod znajduje się w paczce. Co prawda w tej chwili wszystko jest w jednym pliku (dlatego tylko klasa EntryPoint jest publiczna), ale dzieje się tak tylko ze względu na przepełnienie stosu i brak konieczności otwierania Eclipse.
  • Jest JavaDoc wyjaśniający, co się dzieje - przynajmniej w przypadku kilku metod. (Prawdopodobnie dodałbym trochę więcej w prawdziwym kodzie. Kończy mi się czas.)
  • Sprawdzamy poprawność danych wprowadzonych przez użytkownika, z wyjątkiem liczby słów – i przeprowadzamy tę walidację w ramach jednej procedury, która powinna być w rozsądny sposób testowalna. Możemy wtedy założyć, że ilekroć poprosiliśmy o dane wejściowe, otrzymaliśmy coś ważnego.
  • Liczba metod statycznych w EntryPoint jest nieco niepokojąca. Nie wydaje się to strasznie OO - ale często tak jest w przypadku punktu wejścia do programu. Pamiętaj, że nie ma to nic wspólnego z opłatami – to w zasadzie tylko interfejs użytkownika.

Jest tu więcej kodu niż wcześniej - ale jest to (IMO) znacznie bardziej czytelny i łatwiejszy w utrzymaniu kod.

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);
        }
    }
}
person Jon Skeet    schedule 18.10.2009
comment
Nie sądzę, że obie sugestie są dobrymi pomysłami. Ostrzeżenie, że zmienna nie została zadeklarowana, jest ważne, aby zapobiec błędom (niezainicjowanym, odczytaniu). Liczba cyklomatyczna metody wynosi, według moich obliczeń, 7, więc funkcja również nie jest wyjątkowo duża. - person Red33mer; 18.10.2009
comment
Złożoność cyklomatyczna nie jest ostateczna pod względem czytelności. Metoda ma 125 linii i jestem pewien, że bardzo można ją łatwo podzielić. Deklarowanie zmiennych tak późno, jak to możliwe, nie usuwa ostrzeżenia o niezainicjowanych zmiennych - po prostu ułatwia zrozumienie tego ostrzeżenia. Podtrzymuję obie moje sugestie. - person Jon Skeet; 18.10.2009
comment
...a teraz dodaję trzecią. - person Jon Skeet; 18.10.2009
comment
Innym powodem podziału tego jest testowalność. Obecna metoda jest w zasadzie nietestowalna. (Tak, mógłbyś to zrobić, ale byłoby to szaleństwo.) Podział metody na logiczne części w celu obliczenia każdej wartości oddzielnie od jej danych wejściowych sprawi, że będzie to dużo łatwiej sprawdzić. - person Jon Skeet; 18.10.2009
comment
Dziękuję, Jon. Minął dopiero miesiąc odkąd zacząłem się uczyć. Dlatego niektóre sugestie nadal wydają mi się trochę obce, ale dobrze, że uczę się czegoś nowego. Nadal nie wiem, jak utworzyć wiele metod i jak prawidłowo je podzielić. - person HelloWorld; 18.10.2009
comment
@HelloWorld - nie martw się, dotrzesz tam. Ale poważnie sugeruję, abyś zaczął uczyć się więcej o tworzeniu innych metod i typów, zanim pójdziesz dalej. W szczególności nie próbuj zaczynać prawdziwego programowania w celach przydatnych, dopóki nie poznasz więcej podstaw. - person Jon Skeet; 18.10.2009
comment
Czy pomogłoby, gdybym spróbował pobrać przykładowy kod i go zreformować, aby pokazać, jak mógłby wyglądać? - person Jon Skeet; 18.10.2009
comment
+1 Ponieważ jestem nowy w rdzeniu Java, jestem bardzo wdzięczny za jasne i zwięzłe odpowiedzi. Dodane do zakładek :-) - person Edward J Beckett; 26.10.2012

linkCost nie jest inicjowany, gdy link == 'y' i advPay nie jest 'y' lub 'n'.

Innymi słowy, pojawia się ten błąd, gdy kompilator może znaleźć ścieżkę w kodzie, w której zmienna lokalna nie została zainicjowana przed jej użyciem.

person eljenso    schedule 18.10.2009

dzieje się tak, ponieważ przypisanie występuje wewnątrz warunku. Jeśli warunek nie jest spełniony, przypisanie nigdy nie następuje

aby uniknąć błędu, musisz przypisać wartość (najczęściej będzie to 0) poza warunkiem.

person josemrb    schedule 18.10.2009

Analiza przeprowadzana przez Eclipse w celu ustalenia, czy zmienna jest przypisana do każdej ścieżki kodu, nie jest wystarczająco inteligentna, aby zdać sobie sprawę, że testy na numberOfWords nigdy nie zakończą się niepowodzeniem. Zwykle, ponieważ nie jest możliwe statyczne oszacowanie każdego możliwego warunku, kompilator/kontroler składni nie będzie próbował ocenić żadnego z nich. Jeśli zastąpisz ostatnie „else if” „else”, powinno to zadziałać, ponieważ co najmniej jedno przypisanie do textCost nastąpi niezależnie od testowanych warunków.

person Nicholas Riley    schedule 18.10.2009
comment
Zrobiłem tak, jak zasugerowałeś dla TextCost i zastąpiłem else, jeśli innym. Dzięki. - person HelloWorld; 18.10.2009

Eclipse ostrzega Cię, ponieważ inicjalizacja odbywa się w trybie warunkowym. Jeśli żaden z warunków nie zostanie spełniony, textCost nie zostanie zainicjowany.

if (numberOfWords <= 25)
    {
            //assign a value to textCost
    }
    else if (numberOfWords <= 35)
    {
            //assign a value to textCost
    }
    else if (numberOfWords > 35)
    {
            //assign a value to textCost
    }

Eclipse prawdopodobnie nie rozpoznaje, że (numberOfWords <= 35) i (numberOfWords > 35) obejmują wszystkie możliwości.

Możesz albo zainicjować go na 0 przy deklaracji, albo dołączyć dodatkowy else {}, który ustawia go na zero.

Podobne wyjaśnienie dla drugiej zmiennej.

person hexium    schedule 18.10.2009
comment
Chyba chciałeś powiedzieć (numberOfWords ‹= 35) i (numberOfWords > 35). Kurwa, kopiuj/wklej ;) - person ThisSuitIsBlackNot; 18.10.2009
comment
Dziękuję, też to zauważyłem i już poprawiałem, gdy odpowiedziałeś. :) - person hexium; 18.10.2009
comment
Możesz także zastąpić else if (numberOfWords > 35) samym else. A Eclipse prawdopodobnie nie może rozpoznać, że wszystkie możliwości są uwzględnione; Java faktycznie określiła zasady określające, kiedy emitować a, które mogło nie zostać zainicjowane i w tym przypadku mogło to nastąpić. - person JaakkoK; 18.10.2009

komunikat o błędzie informuje, że te zmienne nie są zawsze inicjowane. Dzieje się tak, ponieważ inicjalizacja odbywa się tylko pod pewnymi warunkami (znajdują się one w instrukcjach if). Mam nadzieję że to pomoże..

person andyp    schedule 18.10.2009

Nawet jeśli wiesz, że zostanie odwiedzona jedna z 3 gałęzi porównania z numberOfWords, kompilator o tym nie wie. Błędnie założy, że możliwe jest wprowadzenie klauzuli else, a zmienna textCost pozostanie ujednolicona.

Podobnie z switch (advPay). Nawet jeśli wiesz, że zostanie odwiedzony jeden z dwóch, kompilator tego nie robi.

Sugestia: usuń else if (numberOfWords > 35) i zmień go na else.

Jeśli chodzi o switch (advPay), dodaj przypadek default. Wewnątrz możesz umieścić throw new AssertionError();.

person RichN    schedule 18.10.2009
comment
Czy byłoby lepiej, gdybym po prostu usunął instrukcję switch i po prostu utworzył zagnieżdżoną instrukcję if? - person HelloWorld; 18.10.2009
comment
To naprawdę nie robi żadnej różnicy. Jednak nadal potrzebujesz klauzuli else. - person RichN; 18.10.2009

Dobrym sposobem na uniknięcie takich problemów jest ustawienie zmiennej, która ma zostać przypisana, na final i niezainicjowana przed sprawdzeniem. Zmusi Cię to do ustawienia wartości, zanim będziesz mógł jej użyć/odczytać.

final textCostTmp;
if (condition1) {
  textCostTmp = ...;
} else if (condition2) {
  textCostTmp = ...;
} 
// if you use textCostTmp here the compiler will complain that it is uninitialized !
textCost = textCostTmp;

Aby rozwiązać ten problem, NIE inicjuj zmiennej, ponieważ możesz pominąć brakujący przypadek else. Jedynym właściwym rozwiązaniem jest dodanie przypadku else, aby uwzględnić wszystkie możliwe przypadki! Uważam za złą praktykę inicjowanie zmiennych innych niż końcowe, z wyjątkiem niektórych rzadkich scenariuszy, takich jak licznik w pętli.

Proponowane podejście wymusi na Tobie radzenie sobie ze wszystkimi możliwymi przypadkami, teraz i w przyszłości (łatwiejsze w utrzymaniu). Kompilator jest czasami trochę głupi (nie mogę zrozumieć, że liczba słów > 35 to coś innego)...ale kompilator jest Twoim sprzymierzeńcem, a nie wrogiem...

person Christophe Roussy    schedule 11.08.2014