wiki:harjoitustyo/vikoja
Last modified 2 years ago Last modified on 2015-12-26 23:53:05

Ohjelmointi 2, Tyypillisiä harjoitustöissä olevia vikoja

Tarkistakaa, ettei omissa harjoitustöissänne ole mm. seuraavia vikoja:

Tarkista että työ on oikein netissä

  • kaikki tiedostot ovat versionhallinnassa (menkää "tyhjälle koneelle" ja ajakaa svnht ja katsokaa saatteko tehtyä toimivan Eclipsen projektin)
  • työ näkyy myös https://www.mit.jyu.fi/demowww/ohj2/ht15/OMATUNNUS/vaihe2/
  • svnht-komennon käyttämiseksi pitää muistaa päivittää aina files.txt
  • .java tiedostot pitää olla pakettia vastaavassa hakemistossa ennen svnht-käyttöä ja files.txt:ssä pitää olla vastaava polku (erityisesti /-merkillä)
    src/kerho/Jasen.java    (Eclipsen myötä nykyisin mielellään näin)
    
    muuten ei tule dokumentteja eikä värillistä versiota.
  • myös ohjelman ajon vaatimat data-tiedostot ovat versionhallinassa (niin että kun Eclipse projekti tehdään, on tiedostot oikealla paikallaan heti svnht:n jälkeen)
  • versionhallinnasta tulee tiedostoja, jotka eivät ole enää käytössä

Suunnitteluvaiheen ongelmat

  • ei ole mietitty, kuka käyttää ohjelmaa ja mitä toimintoja hän tarvitsee (=käyttäjätarinat)
  • Kaikkia toimintoja ei ole kuvattu
  • puuttuu OIKEAT esimerkit, ei saa olla joku1, joku2, joku3
  • puuttuu tiedostojen esimerkit
  • tiedostoja on vain 1 kpl (pitää olla VÄHINTÄÄN 2, mieluusti ehkä 3, ei kuitenkaan kymmeniä)
  • tiedosto-esimerkeissä EI OLE dataa
  • tiedosto-esimerkeissä on dataa tyyliin joku1, joku2, joku3. Pitää olla oikeata dataa.
  • tiedostoesimerkeissä on liian vähän dataa (eli rivejä, vähimmilläänkin rivejä pitäisi olla 3-5 ja relaatiolla olevissa tiedostoissa likemmäksi 10, jotta voi harjoitella algoritmejä)
  • suunnitelman käyttöliittymän kuvista puuttuu oikean datan tekstit. Jos suunnitelman tiedostossa on vaikka henkilö Ankka Aku, pitäisi tämän myös näkyä käyttöliittymäkuvassa. Katso: mallisuunnitelmaa

Tietorakennekuvan ongelmat

  • vertaa oikea kuva ja virheellinen kuva
  • kuvasta puuttuu nuolien suunnat
  • nuolet eivät vastaa Javan viitteitä
  • kuvan olioissa ei ole esimerkkidataa
  • kuvissa on ... merkityksellisissä kohti (voi nippa nappa olla jos jäsenellä on "itsestään selviä" attribuutteja jotka eivät vaikuta algoritmeihin).
  • kuvissa ei ole riittävästi olioita (pitäisi olla sen verran että kuvasta voi katsoa mitä etsiminen tarkoittaa)
  • kaikissa rakenteissa on saman verran olioita (pitäisi näyttää että jokaisessa voi olla eri määrä)
  • kuvasta puuttuu esim. Jasenet-luokan kohdalta käytössä olevien alkioiden lukumäärä
  • olisi hyvä että kuvasta löytyisi jokainen merkittävä numero ja teksti joka on suunnitelman mallitiedostoissa. Esim. jos mallitiedostossa on että harrastus on alkanut 1950, niin tuo sama 1950 löytyy kuvasta.
  • oliolaatikoiden sisällä tekstiä jotka eivät kuulu olioon (esim. teksti nimi tai Jasen, tällaiset selittävät tekstit voivat olla laatikon ulkopuolella)
  • taulukoissa dataa vaikka taulukoissa voi olla vain viitteitä olioihin (jollei int tai double-taulukko)
  • monipäisiä nuolia (tällaisia ei Javassa ihan helpolla tehdä)
  • palautettu vain orginaali tiedosto (esim. Word, Excel, Visio tms.) jota ei kaikki pysty lukemaan. Palautettava myös .png tai vastaava (export).

Luokkien vastuu-ongelmat

  • luokkien vastuut ovat väärin, esim:
    • muu kuin käyttöliittymäluokka hoitaa tulostuksen
    • System.out (tai liian moni System.err) ei saa löytyä muulta kuin käyttöliittymäluokasta/luokista
    • konkreettista etsintää tekee käyttöliittymäluokka
  • muodostajan ei olisi hyvä lukea tiedostoa, vaan muodostajan tehtävä on alustaa olio minimaaliseen toimivana kuntoon ja sitten metodeilla jatketaan
    VÄÄRIN: 
      Kerho kerho = new Kerho("kelmit.dat"); /// lukee tiedoston samalla
    
    OIKEIN:
      Kerho kerho = new Kerho(); // luo tarvittavat toimivat rakenteet
      ...
      try { 
         kerho.lueTiedosto("kelmit"); // lukee tiedostot ja heittää poikkeuksen 
                                      // jos joku menee pieleen
      catch ( ... 
    
  • kuljetellaan olioiden välistä dataa pitkissä merkkijonoissa
    VÄÄRIN:
      sb.append(nimi+"|"+"hetu+"|"....
      kerho.lisaa(sb.toString());
    
    OIKEIN (käyttöliittymässä):
      Jasen jasen = hommaa käsiteltävä jasen
      ... keskustelu suoraan jäsenen kanssa ...
      kerho.lisaa(jasen);
        tai
      kerho.korvaa(jasen);
    
  • pitkää merkkijonoa saa käyttää vain kun se on luettu tiedostosta (ja silloin Jasenet ei tiedä sen sisällön merkitystä vaan antaa sen sellaisenaan Jasen-luokan oliolle)

Poikkeusten käsittely-ongelmat

  • esimerkiksi tiedostojen käsittelyssä tapahtuvat virheet eivät nouse käyttöliittymän tulostettavaksi
  • tiedosto jäävät sulkematta (finally puuttuu)
  • ei saa heittää tai ottaa kiinni Exception-luokan poikkeusta. Aina heitetään joku peritty poikkeus ja otetaan kiinni sellaisia poikkeuksia, joille tiedetään mitä tehdään

Tietorakenneongelmat

  • ei ole tehty yhtä omaa rakennetta ja yhtä Collection sukulaista (vaan kaikki esim vaan toisella)
  • lisääminen ei kasvata tilaa tarvittaessa
  • poistaminen älyttömällä algoritmilla
  • poistaminen ei nollaa loppuun jääviä ylimääräistä viiteitä
  • puuttuu joku omatekemä rakenne (vrt. TaulukkoGen.java) tai valmis Javan rakenne (joku Collection-sukulainen)
  • ei ole kertaakaan käytetty iteraattoreita
  • kirjoitettu koodia, josta ei itse ymmärretä mitään.
  • kannattaa välttää indeksillä läpikäymistä (eli ei kannata uskoa että rakenne on taulukkomainen, tällöin heikentää mahdollisuuksia tietorakenteen vaihtamiseen)
  • Taulukoista puuttuu relaatiot

ID:t ja indeksit

  • ID:t ja ja indeksit sekaisin, erityosesti ei saa luulla että id:llä ja indeksi on joku suhde (esim i=id-1) koska oliota voi olla poistettu välistä ja silloin id:t eivät ole juoksevia
  • ID:n käsittely perustuu peräkkäisyyteen (pitää voida puuttua ID-numeroita välistä)
  • ID:t eivät toimia enää tiedoston lukemisen jälkeen ("automaattinumerointi" antaa samoja numeroita joita jo on käytössä)
  • muodostajassa rekisteröiminen helposti käyttää ID:tä liikaa (tällöin pitää olla vähintään kaksi muodostajaa, joista toinen rekisteröi ja toinen ei)

Nimeämis- ja kommentointiongelmat

  • metodit pitää nimetä niin, että niiden nimestä voi heti päätellä mitä metodi tekee
  • nimessä turhia sanona, esim: lisaaJasen(jasen) vaikka pelkkä lisaa(jasen) ajaa saman asian
  • JavaDocit pitää olla kirjoitettuna
  • testit pitää olla riittävän kattavat
  • nimet väärin, esim. Lisaa(jasen) vaikka Javssa on sovittu, että metodien nimet pienellä ja luokkien isoilla
  • metodin kommenteissa ei kerrota kaikkia metodin sivuvaikutuksia, esim. metodi poista, jossa ei kuitenkaan ole dokumentoitu että poisto myös sotkee järjestystä (jos poisto toteutetaan niin että viimeinen alkio siirretään poistettavan kohdalle)
  • WindowBuilderin antamat nimet täytyy vaihtaa. Koodiin ei siis saa jäädä nimiä tyyliin panel, panel_1 jne, vaan esim panelJasenenTiedot jne.

Koodausongelmat

  • luodaan turhia olioita:
      ArrayList<Henkilo> loytyneet = new ArrayList<Henkilo>();
      loytyneet = arkisto.etsiNimet(valinta);
    
    eli tuo = new ArrayList<Henkilo>(); on väärin, koska etsiNimet luo ja palauttaa ko listan ja tällöin new:llä luotua kukaan ei koskaan ehdi edes käyttämään.
  • hirveästi static-aliohjelmia (olio-ohjelmointiin ei kuulu static, mutta joskus joku
       public static int pyorista(double d)
    
    toki voi olla, koska tuo funktio saa kaikki hommansa tehtyä vain parametreillään
  • static attribuutit on kielletty!!! (vain tyyliin Jasen-luokan seuraavaID voi olla static). Tosin seuraavaID-käytössä useimmiten vika ettei selviä tilanteesta, jossa on esim. tiedosto
    5|Ankka Aku|...
    6|Ankka Roope|...
    3|Ankka Lupu|...
    
    eli tiedoston id.t eivät ala 1:stä, eivätkä ole järjestyksessä
  • public attribuutit on kielletty!!!
  • turhia lippumuuttujia:
    public boolean poista(int  id){
        boolean poistettu = false;
        for(int i = 0; poistettu == false && i < this.elokuvia; i++){      
           if (elokuvalista[i].getId() == id){
               elokuvalista[i] = elokuvalista[this.elokuvia-1];
               elokuvalista[this.elokuvia-1] = null;
               this.elokuvia--;
               poistettu = true;
           }
        }
        return poistettu;
    }
    
    kun riittäisi (edellä mainittuhan ei kuitenkaan osaisi poistaa montaa kerralla vaikka for:in ehtoa muutettaisiinkin):
    public boolean poista(int  id){
        for(int i = 0; i < this.elokuvia; i++){      
           if (elokuvalista[i].getId() == id){
               elokuvalista[i] = elokuvalista[this.elokuvia-1];
               elokuvalista[this.elokuvia-1] = null;
               this.elokuvia--;
               return true;
           }
        }
        return false;
    }
    
  • horjuvaa this-käyttöä - edellä hyvä esimerkki sekavasta käytöstä, joko this kaikkiin attribuutteihin tai ei mihinkään
  • estetään olioiden muuttuminen roskaksi (esim. jos edellisestä esimerkistä puuttui viimeisen laittaminen null:iksi, se ei pääsisi muuttumaan roskaksi)
  • O(n2) algoritmeja vaikka selvittäisiin vähemmällä. Esimerkiksi tyypillisesti poisto tehdään väärin algoritmilla, joka jokaisen esiintymän kohdalla siirtelee muita taaksepäin. Poiston voi helposti tehdä O(n) vaikka poistettaisiin montakin alkiota kerralla.
  • yhdistellään silmukassa merkkijonoja + -operaattorilla, jolloin syntyy kamalasti uusia olioita. Oikea tapa on käyttää vaikka StringBuilderiä ja appendilla lisätä.

Tietorakenteen turha antaminen kaikille luokille

  • esim. tarvitaan käyttöliittymän AvaaTiedostoDialogi-luokassa KerhoSwing-luokkaa
    public class AvaaTiedostoDialogi extends JFrame{
       private KerhoSwing kerhoSwing = new KerhoSwing(); /// PAHASTI VÄÄRIN
       // Nyt syntyisi toinen kerhoSwing jolla ei ole mitään yhteistä
       // alkuperäisen KerhoGUI:ssa luodun kanssa.
       ...
    
  • yhtä VÄÄRIN on yrittää muuttaa KerhoGUI:ssa tuo KerhoSwing staattiseksi ja julkiseksi ja antaa sitten muiden käyttää sitä.
  • jos tällainen systeemi on PAKKO rakentaa, niin ensinnäkin pitää miettiä, että tarvitsevatko muut luokat todella tuota KerhoSwing-oliota vai Kerho-oliota? Jos joku todella perustellusti tarvitsee, niin tarvittava olio viedään lomakkeen luonnin parametrina tyyliin:
    KerhoGui:
    ...
        avaaDialogi = new AvaaTiedostoDialogi(kerhoSwing); /// viedään viite parametrina
    ...
      ja vastaavasti olio otetaan vastaan {{{AvaaTiedostoDialogin}}} muodostajassa
      (vertaa {{{Kissa}}} ja {{{Koira}}} -luokkien luominen ja niille tiedon vieminen
      muodostajassa).
    
  • tämä ratkaisu kuitenkin johtaa tilanteeseen, missä jos luokista piirrettäisiin riippuvuuskuva niin siitä tulee helposti kehä!
  • pahin mahdollinen tilanne tulee jos tuo toinen dialogi sitten vielä kutsuisi KerhoSwingin metodia, joka taas ehkä lopulta kutsuisi uudelleen AvaaTiedostoDialogia...
  • parempi on yrittää päästä riippuvuuksista eroon, ks esim:
  • vastaavasti voitaisiin tarvittaessa tehdä vaikka KysyJasen-dialogi, joka saisi viitteenään vain yhden jäsenen tiedot ja sitten kun homma on valmis, niin jatkotoimet jäsenelle hoitaisi se joka tuota dialogia on kutsunut (esim. siis KerhoSwing). Näin KysyJasen-dialogin ei tarvitsisi tietää muusta systeemistä (kuin Jasen-luokasta) yhtään mitään.

Vastuut väärin

  • jos metodista 90% ei koske yhteenkään omaan attribuuttiin, vaan komentelee koko ajan jotakin toista oliota, on vastuut väärin. Tällöin tuo koodi pitää siirtää sinne vastaavaan olioon.
  • Tyyppiesimerkki tästä on tiedon siirtäminen tiedostosta luetusta rivistä itse olioon (esim. jasen). Jos Jasenet-luokassa lukee
      String rivi = ... tiedostosta luettu rivi ... 
      Jasen jasen = new Jasen();
      String[] osat = rivi.split("\\|");    
      jasen.setId(osat[0]);
      jasen.setNimi(osat[1]); // mistä voi edes tietää että pala 1 on olemassa???
      ... 
    
    on Jasenet-luokassa tehty selkeästi Jasen-luokan tehtäviä. Tämän pitää olla:
      String rivi = ... tiedostosta luettu rivi ... 
      Jasen jasen = new Jasen();
      jasen.parse(rivi); // ja täälläkin split ei ehkä ole hyvä vaihtoehto
    

Rekursio: tapahtumat laukovat toisiaan

  • Otetaan esimerkki:
       public String asetaVastustajaksi(){
           ...
             int valittu = listPelit.getSelectedIndex();
             if (valittu>=0){
                listPelit.setSelectedIndex(valittu);
              } else {
                listPelit.add(ottelu.pelinTunnus(peliKohdalla), peliKohdalla);
                hae(listPelit.getSelectedIndex());
             }
           ...
        }
    
    aiheuttaa turhaan rekursion, koska tuo listPelit selectedChange -tapahtuma on laitettu asettamaan vastustaja. Silloin myös
        listPelit.setSelectedIndex(valittu);
    
    aiheuttaa tuon saman selectedChange tapahtuman ja siitä taas tullaan tuohon asetaVastustajaksi uudelleen.
  • Kysymys onkin, että jos tuo valittu on jo kohdallaan, niin miksi silloin pitää se laittaa uudelleen? Oikeastaanhan kyseessä on valitun tapauksessa koodi:
          int valittu = listPelit.getSelectedIndex();
          listPelit.setSelectedIndex(valittu);
    
    Tällä siis oikein pakotetaan tapahtuma laukeamaan uudelleen.
  • Eli taas lyhyempi koodi on selkeämpi ja nätimpi:
        public String asetaVastustajaksi(){
           ...
             int valittu = listPelit.getSelectedIndex();
             if ( valittu < 0 ) {
                listPelit.add(ottelu.pelinTunnus(peliKohdalla), peliKohdalla);
                hae(-1);
             }
           ...
        }
    
  • Tosin en ota kantaa siihen, missä tuo uusi lisätään varsinaiseen rakenteeseen. Mullahan lopullisessa mallissa uutta ei lisätä listaan ennenkuin editointi päättyy.

Koodissa paljon virheitä tai varoituksia

  • koodissa ei saa olla yhtään virhettä tai varoitusta asetuksilla, jotka on neuvottu laittamaan Eclipsen ohjeessa