Ohjelmointi 2, Tyypillisiä harjoitustöissä olevia vikoja

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

1. Tarkista, että työ on oikein netissä

• kaikki tarvittavat tiedostot (mutta ei turhia) ovat versionhallinnassa (menkää "tyhjälle koneelle" ja ajakaa clone ja katsokaa saatteko tehtyä toimivan Eclipsen projektin)

• java-tiedostot pitää olla pakettia vastaavassa hakemistossa

   src/kerho/Jasen.java    (Eclipsen myötä nykyisin mielellään näin)

• myös ohjelman ajon vaatimat data-tiedostot ovat versiohallinnassa (niin että kun Eclipse projekti tehdään, on tiedostot oikealla paikallaan heti clonen jälkeen)

• versiohallinnasta ei saa olla tiedostoja, jotka eivät ole enää käytössä

• olet mergennyt ja commitoinut masterin ja pushannut

2. 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 tiedostojen esimerkit, eli ei ole mallia vastaavia nimet.dat ja harrastukset.dat jne.
• jokainen toiminto pitää olla kuvattu, myös menun toiminnot.
• tiedostoja on vain 1 kpl (pitää olla VÄHINTÄÄN 2, mieluusti ehkä 3, ei kuitenkaan kymmeniä)
• tiedostojen välillä on pelkkä yhdestä-yhteen-relaatio
• tiedostoesimerkeissä EI OLE dataa
• tiedostoesimerkeissä on dataa tyyliin joku1, joku2, joku3. Pitää olla oikeata dataa. Edes tyyliin Jaska Jokunen, Aku Ankka
• 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

3. Vaihe 3:n ongelmat

• itse tehdyt dialogit eivät aukea

• käytetty yhtä kontrolleria useammassa eri näkymässä.

  • sinänsä ok, mutta monesti joutuu korjaamaan jälkeenpäin, eikä tule ilmi ennen ht7 vaihetta, kun keskitytään tietorakenteeseen ja pääikkunaan.

• alimmainen dialogi ei ole pääikkuna (korttipakkamainen käyttöliittymä).

  • suunnitelma muistuttanut enemmän nettisivua tai puhelimen sovellusta, jossa ei tarvitse peruutella.
  • harvemmin toteutetaan dynaamista fxml-näkymän ja kontrollerin vaihtamista, kuten ht2 vaiheessa ehkä suunniteltu (modaalinen sovellus ja 10 ikkunaa).

• Ongelma: dynaamiset näkymät ja kontrollerit avataan aina uuteen ikkunaan, jolloin ikkunoissa toistuvat osittain samat elementit ja koodit.

  FXMLLoader ldr = new FXMLLoader(getClass().getResource("view.fxml"));
  Pane root = (Pane) ldr.load();
  Controller ctrl = (Controller) ldr.getController();
  
  ctrl.setKerho(kerho);
  
  Stage window = (Stage) viewContainer.getScene().getWindow();
  window.setScene(new Scene(root));

  Ratkaisu: dynaamiset ikkunat voivat sisältää useampia näkymiä ja kontrollereita.

  FXMLLoader ldr = new FXMLLoader(getClass().getResource("view.fxml"));
  Pane root = (Pane) ldr.load();
  Controller ctrl = (Controller) ldr.getController();
  
  ctrl.setKerho(kerho);
  
  viewContainer.getChildren().clear();
  viewContainer.getChildren().add(root);

4. 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).

5. 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)

6. Poikkeusten käsittely-ongelmat

• esimerkiksi tiedostojen käsittelyssä tapahtuvat virheet eivät nouse käyttöliittymän tulostettavaksi
• tiedostot 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

7. Tietorakenneongelmat

• ei ole tehty yhtä omaa rakennetta ja yhtä Collection sukulaista (vaan kaikki esim vaan toisella). Harjoituksen vuoksi pitää tehdä molemmat!
• 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

8. 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 toimi 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)

9. Nimeämis- ja kommentointiongelmat

• metodit pitää nimetä niin, että niiden nimestä voi heti päätellä mitä metodi tekee
• nimessä turhia sanoja, esim: lisaaJasen(jasen) vaikka pelkkä lisaa(jasen) ajaa saman asian
• JavaDocit pitää olla kirjoitettuna
  • ei riitä @param nimi vaan pitää kertoa myös mikä on nimi-parametrin merkitys
  • pitää olla @return myös kommentoituna (pitäkää ne Eclipsen virheet kommenteista päällä!!!)
• testit pitää olla riittävän kattavat
• nimet väärin, esim. Lisaa(jasen) vaikka Javassa on sovittu, että metodien nimet pienellä ja luokkien nimet isolla
• 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.

10. 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ä on useilla vika, ettei koodi 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ä.

11. 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:

  • https://www.mit.jyu.fi/demowww/ohj2/ht13/vesal/vaihe3/

  • https://www.mit.jyu.fi/demowww/ohj2/ht13/vesal/vaihe3/src/wbKerho/KerhonNimiDialog.java

  • https://www.mit.jyu.fi/demowww/ohj2/ht13/vesal/vaihe3/src/wbKerho/KerhoGUI.java

      422     protected boolean avaa() {
      423         // kerhoswing.talleta();
      424         String uusinimi = KerhonNimiDialog.askName(kerhonnimi);
      425         if ( uusinimi == null ) return false;
      426         lueTiedosto(uusinimi);
      427         return true;
      429     }

• jossa siis KerhoNimiDialog ainoastaan kysyy ja palauttaa kerhon nimen. Näin dialogin ei tarvitse tietää muusta maailmasta yhtään mitään ja riippuvuudet vähenevät huomattavasti.

• 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.

12. 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

13. 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.

14. Koodissa paljon virheitä tai varoituksia

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