Changes between Initial Version and Version 1 of Halyri/Koodinkatselmointi2Muistio


Ignore:
Timestamp:
2014-06-10 18:14:37 (5 years ago)
Author:
iltaraut
Comment:

--

Legend:

Unmodified
Added
Removed
Modified
  • Halyri/Koodinkatselmointi2Muistio

    v1 v1  
     1Hälyri-sovellusprojektin 2. koodinkatselmoinnin muistio 
     2 
     3Paikka: Tietotekniikan projektien kokoushuone, Agora AgC226.1 \\ 
     4Aika: Torstai 22.5.2014 klo 8.37–11.00 
     5 
     6Läsnä \\ 
     7Projektiryhmä 
     8- Atte Söderlund 
     9- Ilkka Rautiainen 
     10- Veli-Mikko Puupponen, sihteeri 
     11Tietotekniikan laitoksen edustajat 
     12- Vesa Lappalainen 
     13Ohjaajat 
     14- Jukka-Pekka Santanen (vastaava ohjaaja) 
     15- Jaakko Kosonen (tekninen ohjaaja) 
     16 
     17Muistio 
     18 
     19Laadittu: 26.5.2014 
     20Muokattu: - 
     21 
     22 
     231. Hätäkeskusohjelma 
     24 
     25Hätäkeskusohjelman uusimmasta versiosta käsiteltiin ohjelman päänäkymän lähdekooditiedostoa Mainwindow.xaml.cs, EKG-dataa visualisoivan luokan lähdekooditiedostoa GraphClass.cs, tehtävälistan elementtiluokan lähdekooditiedostoa Assignment.cs ja verkkoliikennettä hoitavan luokan lähdekooditiedostoa Connection.cs. 
     26 
     27 
     28Mainwindow.xaml.cs  
     29 
     30Lähdekooditiedostossa Mainwindow.xaml.cs Kosonen ehdotti ottamaan huomioon seuraavat asiat: 
     31-       Ohjelman käyttämän palvelimen oletusosoitteen voisi siirtää App.config-tiedostoon. 
     32-       Näkymän muodostajassa avataan modaalisia dialogeja, tehdään paljon tarkistuksia ja kutsutaan monia näkymään liittymättömien elementtien muodostajia. Näistä osa kuuluisi luultavasti jo ohjelman Application-luokan metodeihin, osa vasta näkymän muodostamisen jälkeen kutsuttaviin metodeihin. 
     33-       Luokka- ja metodikommentit ovat lähdekoodissa, mutta pidempien metodien sisällä kannattaisi käyttää ainakin jatkossa myös rivikommentteja. Niitä ei kuitenkaan enää tarvitse lisätä tämän projektin toteutettuun koodiin. 
     34 
     35 
     36Santasen huomioita olivat: 
     37-       Rivit ovat osittain liian pitkiä. Tämä tulee vaatimaan rivien pituuden rajoittamista tässä ja myös muissa lähdekooditiedostoissa viimeistään projektikansiota koottaessa. 
     38-       Kaikkien dialogien tekstit eivät nyt pääty päätemerkkiin. Tämä kannattaa korjata ennen hyväksytettävää versiota. 
     39-       Dialogien tekstit näyttävät olevan kovakoodattuja. Tämä tekee uusien kieliversioiden tuottamisen vaikeaksi. 
     40 
     41Lappalainen: 
     42-       Ohjelman asetusdialogissa olisi hyvä olla mahdollisuus muuttaa palvelimen käyttämää palvelinosoitetta. 
     43-       Ping-viestejä palvelimelle lähettävälle luokalle muodostajassa asetettava aikaväli tulisi esitellä luokan alussa const-muuttujana. Santanen oli samaa mieltä. 
     44-       Dialogien tekstit ovat kovakoodattuina hankalia muuttaa esimerkiksi kieliversioita ajatellen. Söderlund kuitenkin totesi, että kieliversiot tuotetaan tässä vaiheessa vain mobiilisovelluksesta. 
     45-       Metodi Connection_AssigmentUpdatedEvent suhteellisen pitkä ja monimutkainen. Lisäksi se sisältää sekaisin sekä datan lukemista että esittämistä. Tätä ei projektin puitteissa kannata kuitenkaan lähteä enää muuttamaan, mutta muutostarve kannattaa kirjata koodin kommentteihin. 
     46-       Onko useimpien ilmoitusten esittämisessä käytetty modaalinen dialogi paras vaihtoehto? Parempi ratkaisu voisi olla esimerkiksi rajapinta viestin esittämistä varten. Tässä vaiheessa sen ainoa toteutus voisi olla modaalinen dialogi, mutta myöhemmin sitä voitaisiin laajentaa tarjoamaan myös erillinen viestialue käyttöliittymään. Tätä muutosta ei kuitenkaan projektin aikana kannata enää lähteä toteuttamaan. 
     47-       Monille toiminnoille käytetään painiketta, jonka sisältötekstiä vaihdetaan taustalla koodissa. Tämä tekee uusien kieliversioiden tuottamisesta erittäin hankalaa. Parempi ratkaisu olisi esimerkiksi käyttää kahta päällekkäistä painiketta, jotka sisältävät valmiiksi aiotut tekstit. Näin selvittäisiin vain painikkeiden näkyvyyttä vaihtamalla. Tätä muutosta ei kuitenkaan kannata projektin kuluessa enää toteuttaa, mutta se kannattaa kirjata ylös lähdekoodin ja mahdollisesti sovellusraporttiin. 
     48-       Yleisen poikkeuksen kaappaaminen lähetettäessä EKG-datan lopettava pyyntö metodissa CancelMeasurementDataFromDevice_Click ei ole hyvä ratkaisu. Vähintään olisi kirjoitettava lokiin mitä tapahtui. Tätä ei tarvitse enää muuttaa, mutta kaikki sellaiset koodikohdat, joissa yleisiä poikkeuksia otetaan kiinni ja jätetään käsittelemättä, kannattaa merkitä koodin kommentteihin kehitystä kaipaaviksi. 
     49-       Monissa metodeissa tehdään tarkistuksia null-argumenttien varalta. Ehdollinen haarautuminen vaatii vähintään yhden uuden testitapauksen, mutta pahimmillaan kaksinkertaistaa tarvittavien testitapausten määrän. Näitä ei kannata lähteä koodista enää muuttamaan, mutta tämä näkökulma kannattaa ottaa jatkossa huomioon. 
     50-       Painikkeiden sisältötekstiä vaihdetaan useissa metodeissa. Jos tästä toimintatavasta ei luovuta, toiminto voisi olla kuitenkin järkevää siirtää erilliseen metodiin. 
     51-       Metodin ImageHandler merkitys on epäselvä, koska käyttöliittymässä puhutaan videosta ja esitetään liikkuvaa kuvaa. Metodin kommentti ja mahdollisesti myös sen nimi kannattaa muuttaa vastaamaan sen merkitystä älypuhelinsovelluksen lähettämän liikkuvan kuvan esittäjänä. 
     52-       Älypuhelinsovelluksen lähettämän kuvan esittämisessä käytetään kierto- ja siirtomuutoksia. Jos kiertoja tehtäisiin yhtään useammin, kannattaisi muutokset muodostaa vain kerran ja tallentaa järjestetyksi neljän muunnoksen taulukoksi. Nyt muunnoksia kuitenkin tehdään niin harvoin, että tämä muutos riittää kirjata ylös lähdekoodin dokumentaatioon. 
     53 
     54Lähdekooditiedoistoista GraphClass.cs, Assignment.cs ja Connection.cs Kosonen esitti seuraavat huomiot: 
     55-       GraphClass-luokan kommentteihin kannattaa lisätä maininta sen tekijästä. 
     56-       Connection.cs:ssä on joitain pitkiä rivejä, jotka kannattaa rivittää jo selkeydenkin kannalta. 
     57-       Erityisesti Assignment-luokka olisi sisältänyt monia yksikkötestattavia osia. Tämä kannattaa ottaa jatkossa huomioon ja kirjata ylös ainakin sovellusraporttiin. Myös Lappalainen huomautti yksikkötestauksen puuttumisesta. 
     58-       Tuotaessa palvelimen WSDL-kuvaus projektiin saadaan LocationDto-säilöluokka partial-tyyppisenä. Näin siihen voi laajentaa omia metodeja, jolloin koodissa toistettavat muunnokset erilaisten paikkatietoluokkien välillä voidaan sijoittaa sinne. Muutosmahdollisuus kannattaa kirjata ainakin koodin kommentteihin. 
     59-       Assignment-luokassa haetaan paikkatiedoilla verkkopalvelusta asynkronisesti paikkakunta ja osoite. Nyt käytettävä tapahtumiin perustuva ratkaisu on monimutkainen. Yksinkertaisemmin selvittäisiin avaamalla await-avainsanalla uusi säie odottamaan asynkronisen kutsun valmistumista. 
     60 
     61Lappalaisen ehdotuksia olivat: 
     62-       Assignment-luokka sisältää RGB-arvoina määriteltyjä värivakioita, joita ei ole millään tavalla kommentoitu. Värit kannattaa esitellä const-tyyppisinä jo luokan alussa ja niiden käyttötarkoitukset sekä väri on kuvattava kommenteissa. 
     63-       SetPushpin on monimutkainen metodi. Kannattaisiko karttamerkit muodostaa jo Assignment-oliota luotaessa tai siirtää ainakin niiden muodostaminen erilliseen tehdasmetodiin Assignment-luokassa?  
     64-       Ehtolauseita on käytetty monissa metodeissa tarpeettomasti. Tällaisia metodeja ei kannata projektin piirissä lähteä muokkaamaan, mutta niiden muutostarve on hyvä kirjata kommentteihin. Jatkossa kannattaa aina muistaa ehtolauseiden vaikutus testitapausten määrään. 
     65-       Tehtävälistassa esitettävien koordinaattien esitysmuoto on toteutettu rivillä 172 merkkijonovakioilla. Nämä vakiot on syytä esitellä const-tyyppisenä luokan alussa. 
     66-       Haettaessa paikkatiedoilla paikkakuntaa olisi hyvä esittää tehtävälistan elementissä ja tehtävänäkymässä maininta tietojen hakemisesta. Tämä voitaisiin hoitaa yksinkertaisesti korvaamalla osoite esimerkiksi tekstillä ”Haetaan”. Muutosta ei kuitenkaan tarvitse tehdä projektin kuluessa, mutta se kannattaa kirjata kehitysehdotuksiin. 
     67 
     682. Älypuhelinsovellus 
     69 
     70Älypuhelinsovelluksesta käsiteltiin pysäytyskuvia kaappaavaa luokan lähdekooditiedostoa PreviewImageCapturer.cs. 
     71 
     72Lappalainen esitti siitä seuraavat huomiot: 
     73-       ”Source cannot be null” ei ole kuvaava virheteksti. Se kannattaa muuttaa muotoon, jossa kysymyksessä oleva AudioVideoCaptureDevice-luokan olio tulee esille. 
     74-       Luokassa on käytetty asetusmuuttujien suojaamiseen lukkoa. Lukkiuman riskiä ei muutoin välttämättä ole, mutta lukkoa vapauttamatta tehdään myös kuvan muodostaminen käyttöliittymäsäikeessä. Tähän liittyy lukkiuman riski. Koodia ei kannata muuttaa, mutta muutosmahdollisuus kannattaa kirjata kommentteihin. 
     75-       Kuvia kaapataan halutulla kuvanopeudella laskemalla saatuja kuvia ja ohittamalla osa niistä. Jos kuvia tuottavan lähteen kuvanopeus kuitenkin laskee, saadaan kuvia samassa suhteessa harvemmin. Tämä ei-toivottu käytös voidaan välttää, jos kuvien laskemisen lisäksi seurataan viimeisestä saadusta kuvasta kulunutta aikaa ja pakataan kuva myös, jos tämä aika on kasvanut liian suureksi. Muutosehdotus kannattaa kuitenkin kirjata vain kuvat kaappaavan metodin kommentteihin. 
     76Lisäksi Lappalainen pyysi projektiryhmää kokeilemaan, saadaanko sovellus Windows Storeen. Hän myös ehdotti muuttamaan sovelluksen kuvaketta niin, ettei sen taustana käytetä puhelimessa valittua korostusväriä, vaan sovelluksen kiireellisen yhteyden avauspainikkeen keltaista taustaväriä. 
     77 
     783. Palvelinohjelmisto ja UDP-toteutus 
     79 
     80Palvelimen uusimmasta versiosta tarkasteltiin UDP-protokollalla siirrettävän äänen ja kuvan välityspalvelimen toteuttavan luokan lähdekooditiedostoa UdpMediaRelayServerCore.cs. Äänen ja kuvan siirtoon tarkoitetun UDP-asiakaskirjaston projektista tarkasteltiin lähdekooditiedostoja NetworkPacket.cs ja UdpMediaClientSocket.cs. 
     81 
     82Kososen huomioita tästä luokasta olivat: 
     83-       Luokka UdpMediaRelayServerCore ja sitä käyttävä singleton-luokka DataTransferController.cs sisältävät destructorit. Destructoreja ei kuitenkaan pitäisi käyttää hallitun koodin tasolla olevien resurssien vapauttamiseen. Niinpä UDP-socketin vapauttaminen kannattaa hoitaa palvelimen Global.asax.cs-tiedoston metodissa Application_End. 
     84-       NetworkPacket on rajapinta, joten sen nimen pitää alkaa I-kirjaimella. 
     85-       Luokassa UdpMediaRelayServerCore lopetetaan sen käyttämät sisäiset säikeet tarpeettoman suoraviivaisesti Abort-metodilla. Säikeiden oikeaoppiseen lopettamiseen kannattaa perehtyä, mutta projektin puitteissa riittää kirjata metodin kommentteihin menetelmän kehittämistarve. 
     86 
     874. Katselmoinnin päättäminen 
     88 
     89Katselmoinnin lopuksi Santanen kysyi Kososen näkemystä projektissa toteutetuista lähdekoodeista. Kosonen totesi, että lähdekoodit ovat hänen puolestaan kaikkien projektissa toteutettujen järjestelmän osien osalta hyväksyttyjä. Laadullisesti hän piti lähdekoodeja kauttaaltaan hyvinä.