Muistio Sovellusprojekti Verso, 2. koodin katselmointi Paikka: AgC226.2, Agora Aika: tiistai 11.5.2010 klo 15:18–16:50 Muistio laadittu: 14.5.2010 Muistiota muokattu: 17.5.2010 Läsnä Projektiryhmä Tero Hänninen Juho Nieminen Marko Peltola, sihteeri Heikki Salo Ohjaajat Antti-Juhani Kaijanaho Jukka-Pekka Santanen 1. Katselmointitilanne Kaijanaho oli käynyt etukäteen läpi projektiryhmän katselmoitavaksi esittämiä lähdekooditiedostoja. Aluksi Kaijanaho totesi koodin olevan hyvää, eikä siinä esiinny samanlaisia omituisuuksia kuin yleensä opiskelijaprojektien koodeissa. Lähdekoodeista puuttui edelleen kommentit, joista voisi muodostaa Rdoc- dokumentaation. Kommenteissa olevat FIXME-huomiot ovat Kaijanahon mielestä erinomainen tapa merkitä keskeneräiset kohdat. Nämä täytyy luonnollisesti vielä korjata, eikä Kaijanaho katselmoinnissa puuttunut FIXME:llä merkittyihin kohtiin. 2. application_controller.rb Salasanojen suodatus logeista on Kaijanahon mielestä toteutettu erinomaisesti yhdellä rivillä. Komento filter_parameter_logging on tietenkin Ruby on Railsin hienoutta, mutta Kaijanaho kehui projektiryhmää komennon löytämisestä. Metodeissa require_view_right_to_repository ja require_view_right_to_project on metodikutsu redirect_to sisennetty väärin. 3. committerships_controller.rb Kaijanaho oli huomannut, että metodissa destroy käyttäjälle näytetään väärä virheilmoitus adminin poistamisesta, jos committerin poisto epäonnistuu. 4. events_controller.rb Kaijanaho löysi paljon korjattavaa metodissa index olevasta hausta Event.paginate. Koodin sisennys pikemminkin haittasi kuin helpotti luettavuutta. Kaksi alikyselyä voi Kaijanahon mukaan tiivistää yhdeksi alikyselyksi. Lisäksi julkisten projektien lista :pub_projs voi olla todella pitkä, jolloin tietokantahausta tulee hidas. Tämän Kaijanaho korjaisi lisäämällä projektin julkisuustarkastelun itse hakuehtoihin. Kaijanho lupasi toimittaa projektiryhmälle ehdotuksensa siistimmästä koodista katselmoinnin jälkeen sähköpostilla. Santanen kysyi, olisiko kommentointi selventänyt koodia. Kaijanahon mukaan kommentointi olisi auttanut, mutta hänen mielestään parempi olisi kirjoittaa koodi niin selväksi, ettei kommentointia tarvittaisi ollenkaan. Santasen mielestä kannattaisi silti kirjoittaa aina ensin kommentti, ja koodin kirjoittamisen jälkeen tarkastaa, vastaako koodi kommenttia. 5. keys_controller.rb Metodissa index oleva each-metodi olisi Rubymaisempaa korvata any?-metodilla. Näin koodista tulisi lyhyempi ja luettavampi. 6. projects_controller.rb Metodissa show oleva @repositories-listaus olisi kätevämpi tehdä each do -silmukan sijaan metodilla find_all. Kaijanaho suositteli, että jos ei ole varma, miten joku asia pitäisi tehdä, niin API-dokumentaatiosta kannattaa aina katsoa vinkkejä. 7. repositories_controller.rb Metodissa show oli osa riveistä sisennetty liikaa. Hännisen mukaan siinä on saattanut aikaisemmin olla if-ehtolause, jonka poistamisen jälkeen sisennys on jäänyt korjaamatta. Kaijanaho kysyi, miksi merge_requests_enabled -rivi oli poistettu metodista create. Nieminen vastasi, että tämä tieto tallennetaan olioon @repository jo aiemmin. Tämä rivi oli poistettu osana isompaa muutosta, eikä rivin poistosta tullut mainintaa commit-viestiin. Kaijanaho suositteli, että jos yhteen commitiin on tulossa paljon muutoksia, niin esimerkiksi gitk-ohjelman avulla voi tutkia, voisiko commitin jakaa useampaan pienempään commitiin. Kaijanaho oli lisäksi löytänyt create-metodista potentiaalisen tietoturvariskin. Jos jollain on oikeus päästä kirjoittamaan palvelimella tietovarastot sisältävään hakemistoon, niin tietovaraston nimen sopivasti nimeämällä on mahdollista ajaa haitallisia komentoja palvelimella. Tämä sama koodi toistuu metodissa update_repo_with_zip, jolloin kyseisestä koodista olisi syytä tehdä oma metodinsa. 8. sessions_controller.rb Kaijanahon mielestä LDAP-autentikointi olisi syytä laittaa vaihtoehtoiseksi kirjautumistavaksi ja lisätä optio tästä konfiguraatiotiedostoon. Uutta käyttäjää tehdessä käyttäjän tietoihin tallennetaan automaattisesti "terms_accepted". Kaijanaho epäili kyseisen toiminnan laillisuutta, vaikkei käyttöehtotietoja itse sovelluksessa missään näytetäkään. Kaijanahon mielestä kyseisestä toteutusratkaisusta on syytä mainita sovellusraportissa. 9. site_controller.rb Metodissa repositories kohdassa @user_repositories on epäselvä rivitys. Kaijanahon mielestä metodissa create_repo_and_project on paljon tuttua muista metodeista. Näistä kohdista olisi syytä tehdä omia metodeja. 10. trees_controller.rb Kaijanaho kertoi, että vakiintunut tapa ilmoittaa sähköpostiosoitteen olevan toimimaton esimerkkiosoite on laittaa osoitteen domainiksi "invalid". 11. event.rb Metodeissa public_for_site? ja public_for_world? kannattaisi Kaijanahon mukaan jakaa target_type-ehdot erilleen, jotta koodista tulisi luettavampaa. Hänninen ja Nieminen pohtivat, saisiko case-rakenteella kohdasta vieläkin selkeämmän. Lisäksi Kaijanaho oli löytänyt yhden ylimääräisen sulkumerkin metodissa self.latest_in_projects. 12. group.rb Metodissa admins kannattaisi Kaijanahon mukaan käyttää find_all-metodia. 13. project.rb Kaijanahon mukaan publicity-termi ei kuvaa projekteihin liittyvää näkyvyyttä – parempi termi olisi visibility. Projektiryhmä kertoi keskustelleensa kyseisestä termistä jo aikaisemmin ja todenneen saman asian. 14. repository.rb Kaijanahon mielestä tietovarastot ja niiden lisenssit olisi hyvä olla samassa tietorakenteessa, kun nyt ne ovat erillään. Santanen oli samaa mieltä ja toivoi, että tietorakenne mahdollistaisi uusien lisenssien lisäämisen helposti. Metodissa update_contents_from_zip ajettava wget-komento sisältää tietoturvariskin: komennossa oleva source_path voi sisältää shell-komentoja, jotka tulisivat ajettua komentorivillä. Nieminen kertoi, että git svn -komennon kohdalla riski on sama. Metodissa update_contents_from_zip ajettava 'rm -rf *' ei Kaijanahon mukaan poista pisteellä alkavia piilotiedostoja. Komento on hänestä muutenkin arveluttava, vaikkei siitä aiheudukaan mitään vaaratilanteita. 15. user.rb Metodissa ldap_authenticate on myös mahdollista päästä ajamaan komentorivillä komentoja. Lisäksi salasana on kirjautumisen aikana hetken aikaa nähtävillä palvelimella ps-komennon avulla. Kaijanaho suositteli välittämään salasanan autentikointiskriptille stdinin avulla komentoriviparametrin sijaan. 16. gitorious_form_builder.rb Tiedoston tekijänoikeusmerkinnöissä oli pelkästään Juho Niemisen nimi, vaikka tiedosto ei ollut kokonaan hänen tekemänsä. Kaijanahon mukaan tiedoston tekijänoikeuksiin olisi syytä lisätä myös versiohallinnan historiasta löytyvät aikaisemmat kehittäjät. 17. Yhteenveto Kaijanaho toisti vielä, että koodi oli yleisesti hyvää. Santanen kysyi, mitä Kaijanaho vaatii projektiryhmän korjaavan, jotta koodi voitaisiin hyväksyä. Kaijanaho vastasi, että katselmoinnin alussa käyty tietokantakysely ja esiin tulleet tietoturvakysymykset on korjattava. Hän lupasi lähettää sähköpostilla katselmoinnin jälkeen vielä omat muistiinpanonsa, johon hän merkitsee korjattavat kohdat.