Car/Polygon Refactoring

[aether-fox]

Some improvements to the AutomatedCar class, some of its tests and polygon data handling.

Separated polygon coordinates into file, because separating data from code is always beneficial and makes the code more clear. (See added PolygonLoader class.)

[codecov[bot]]

Codecov Report

Merging #187 into master will increase coverage by 1.39%. The diff coverage is 73.33%.

@@             Coverage Diff              @@
##             master     #187      +/-   ##
============================================
+ Coverage     44.72%   46.11%   +1.39%     
- Complexity      275      302      +27     
============================================
  Files            53       57       +4     
  Lines          1878     1958      +80     
  Branches        144      152       +8     
============================================
+ Hits            840      903      +63     
- Misses          990     1004      +14     
- Partials         48       51       +3     

Flag	Coverage Δ	Complexity Δ
#unittests	46.11% <73.33%> (+1.39%)	302.00 <33.00> (+27.00)

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/hu/oe/nik/szfmv/automatedcar/Main.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...v/automatedcar/powertrain/CarTransmissionMode.java	100.00% <ø> (ø)	1.00 <0.00> (ø)
.../nik/szfmv/automatedcar/powertrain/PowerTrain.java	32.25% <ø> (ø)	3.00 <0.00> (ø)
...n/java/hu/oe/nik/szfmv/automatedcar/math/Axis.java	61.81% <33.33%> (-0.45%)	3.00 <0.00> (ø)
...ava/hu/oe/nik/szfmv/automatedcar/AutomatedCar.java	16.98% <66.66%> (-1.54%)	3.00 <0.00> (ø)
.../java/hu/oe/nik/szfmv/automatedcar/CarVariant.java	66.66% <66.66%> (ø)	3.00 <3.00> (?)
...hu/oe/nik/szfmv/automatedcar/math/Coordinates.java	66.66% <66.66%> (ø)	1.00 <1.00> (?)
...ava/hu/oe/nik/szfmv/automatedcar/math/IVector.java	56.00% <71.42%> (ø)	12.00 <3.00> (ø)
...oe/nik/szfmv/automatedcar/model/PolygonLoader.java	80.70% <80.70%> (ø)	18.00 <18.00> (?)
...a/hu/oe/nik/szfmv/automatedcar/math/MathUtils.java	100.00% <100.00%> (ø)	6.00 <5.00> (+2.00)
... and 5 more

[ArchiCat]

Bocs, de ez teljesen felesleges, ráadásul nehezíti a folyamatban lévő munkát.

[ArchiCat]

• Elég komoly api váltásnak tűnik, ez nem fog másnak gondot okozni?

Nekem például egészen biztosan gondot okoz, és nem fogok tudni alkalmazkodni hozzá a megbeszélt feladatnál.

[aether-fox]

Mire gondoltok API változás alatt? Érdemi változtatás az volt, hogy most a polygon adatai fájlból vannak betöltve, nem a kódba vannak szúrva, illetve, hogy ezentúl a konstruktor paraméter értéke egy 3 értékű enum-ból egy érték, nem egy magic string. Szerintem egyik sem hatalmas API change.

Én kicsit azt látom minden hajszálba bele van kötve, miközben a kód tele van tömve kikommentelt nem is használt kódokkal, mezőkkel, "mock" oszátályok örökölnek az eredeti osztályból, úgy hallottam Mockito-t nem is lehet valamiért használni. Vettem a fáradtságot, hogy próbáljam érthető irányba tuszkolni a kódot, de egy metódus paramétertípusának változása, ami effektíve a conflict resolve közben fél perc sincs, az már probléma és hátráltat mindenkit. Elvileg mindenki keni-vágja milyen metákat tárol a git meg a GitHub, ott van az IDE-ben egy menüben egy gombnyomás, persze, de azért a felajánlott 1 klikk quick fix-et se fogadja el a többség.

Nagyon értékelem, hogy nagy figyelmet szenteltek a review-zásra, tényleg. Az szomorít csak el, hogy a kódban ennek nem látom lenyomatát.

(Másik dolog: ha a master frissül, akkor jóvá kell hagyatnom csapattársakkal, hogy a Team3-ba mergeljem. Azon gondolkozom egyszerűbb lenne ha készítenénk magunknak egy "másik Team3" branch-et ami nem protected és azt használnánk. Ez mennyire megengedett?)

[varszegimarcell]

Szvsz sokkal jobb fájlból betölteni a dolgokat, mintsem egy hatalmas magic stringből.

Az hogy teljesen felesleges, oké, de szerintem mások egyszerűbben kiigazodnak a nem "favágó" megoldásokon. A saját kényelmünkkel szemben előnyben kellene részesíteni a kód olvashatóságot, elvégre csapatmunkáról lenne szó.

Amúgy azt a javaslatot szeretném tenni a tantárggyal kapcsolatban, hogy egy kicsit jobban ki kellene hangsúlyozni a tárgy teljesítéséhez szükséges előkövetelményeket, amikor meg van az hirdetve felvételre Neptunban. 0 skillel itt nem lehet felvenni a fonalat... Szerintem ki kellett volna hangsúlyozni, hogy a tantárgy teljesítéséhez, stabil java, illetve git ismeretekre van szükség, valamint megfelelő attitűdre a csapatmunkához.

Azt tapasztalom, hogy a csapatmunka nonegzisztens, a clean kódra teljes igénytelenség van, és pár ember, cobwoy codingban húzza a projektet. Amikor ideülök a projekt elé, semmit sem értek mások munkájából, a javadoc-ot egyszerűen csak egy-két ember használja. És erre azt tudom mondani, hogy ez azért alakult így, mert egy rakás olyan ember is felvette a tárgyat, aki nincs azon a szinten, hogy ebben a projektben megfelelően teljesítsen.

Én C# fejlesztő vagyok az Oktatási Hivatalban, és főleg egy belsős legacy Windows Forms-os alkalmazást foltozgatok a felsőoktatási felvételi háttérszolgáltatásokhoz. Nos, bár ott is igen ocsortány a kód, és gyakran a munkaidőm 90-10% arányban oszlik kódolvasás-fejelsztés felbontásra, az akkor is a munkám. Van hogy igen is hét óra a munkaidőmből kódolvasás, azonban egy egyetemi tantárgy projektjénél, ami mellett van rengeteg egyéb dolgom is, nem fogok ennyi időt szánni a kódolvasásra. Azaz csak akarnék nem szánni, mert vannak akiknek a személyes kényelem fontosabb a clean kódnál. Így nem túl sok mindenhez tudok hozzátenni megmondom őszintén.

Alapvetően szeretek programozni. Akarnék változtatni, és fejleszteni, de bevallom őszintén, miután több hétig szenvedtem 2 approve begyűjtésével, feladtam már ezt az egészet. Egyszerűen nincs kedvem egy olyan projekthez nekiülni fejleszteni, ahol 10 féle képen vagyok kigáncsolva. Gyakran azt érzem, hogy egy szemöldökcsipesszel egyszerűbben tudnék megszabadulni a fanszőrzetemtől, mint hogy itt kijusson a kódom a masterre.

Ami a félév további részét illeti, megteszem amit tudok, de egy könnycseppet nem fogok hullajtani, ha bukás lesz a vége. Ez az egész így nem működik.

[pintergreg]

@aether-fox

Mire gondoltok API változás alatt? Érdemi változtatás az volt, hogy most a polygon adatai fájlból vannak betöltve, nem a kódba vannak szúrva, illetve, hogy ezentúl a konstruktor paraméter értéke egy 3 értékű enum-ból egy érték, nem egy magic string. Szerintem egyik sem hatalmas API change.

Láttam, hogy megváltozik a konstruktora egy olyan osztálynak, ami többeket érint és mindjárt a masterbe szántok. A változás méretétől függetlenül, kapásból az az első gondolatom, hogy erről tudnak-e az érintettek. Konszenzus volt-e váltásban, netalán konkrét kérésre történt. Egy „noticket” munka esetében az ember hajlamos levonni azt a következtetést, hogy nem. Előítélet, persze, tudom...

Én kicsit azt látom minden hajszálba bele van kötve, miközben a kód tele van tömve kikommentelt nem is használt kódokkal, mezőkkel, "mock" oszátályok örökölnek az eredeti osztályból, úgy hallottam Mockito-t nem is lehet valamiért használni. Vettem a fáradtságot, hogy próbáljam érthető irányba tuszkolni a kódot, de egy metódus paramétertípusának változása, ami effektíve a conflict resolve közben fél perc sincs, az már probléma és hátráltat mindenkit. Elvileg mindenki keni-vágja milyen metákat tárol a git meg a GitHub, ott van az IDE-ben egy menüben egy gombnyomás, persze, de azért a felajánlott 1 klikk quick fix-et se fogadja el a többség.

Nem tudom, hogy hol szerepelt az, hogy nem lehet Mock frameworköt használni. Még említésre is kerülnek az előadáson. Laci azt szokta mondani, hogy nem szokott ilyeneket használni, én meg sosem használtam erre framework-öt. Tiltva tuti nincs.

Nagyon értékelem, hogy nagy figyelmet szenteltek a review-zásra, tényleg. Az szomorít csak el, hogy a kódban ennek nem látom lenyomatát.

Megértem. Amikor a távoktatásra átállás közben szétesett az egész projekt egy ponton részben elengedtem minőséget és előrevettem azt, hogy legalább az a pár ember, aki még hétről hétre dolgozik tudjon valami haladni. De ez is csak akkor működik, amikor tudom, hogy ki mit és miért csinál.

(Másik dolog: ha a master frissül, akkor jóvá kell hagyatnom csapattársakkal, hogy a Team3-ba mergeljem. Azon gondolkozom egyszerűbb lenne ha készítenénk magunknak egy "másik Team3" branch-et ami nem protected és azt használnánk. Ez mennyire megengedett?)

Igen, mivel nem tudok rá jobb megoldást. Lobbiztam már a GH-nál, hogy lehessen olyan branch-védelmi szabályt definiálni, amelyben irányultságok is figyelembe vehetők, pl. a masterből, amely elvileg (definíció szerint) csak szép, tiszta, jóváhagyott kódot tartalmaz, ne kelljen újabb review kör. Nem készült el. Egyébként pedig én nem tudok jobb branch stratégiát, amely az oktatási szempontokat is figyelembe véve élhetőbb környezetet biztosít.

Szoktam még mondani, hogy explicit hivatkozz be kommenbe, hogy „@pintergreg ez a masterből megy, told be approve nélkül”. Egész egyszerűen nem kapok notification-t arról, hogy ha csak simán hozzáadsz review-zónak.

A kérdésre válaszolva: nem szoktam ellenezni, ha felgyorsítja a fejlesztést. A Team2-nek is van olyan dev. branch-e (A2Dev), amely a TeamA2 branch védelmi szabályainek megkerülésére született.

@varszegimarcell

Szvsz sokkal jobb fájlból betölteni a dolgokat, mintsem egy hatalmas magic stringből.

Az hogy teljesen felesleges, oké, de szerintem mások egyszerűbben kiigazodnak a nem "favágó" megoldásokon. A saját kényelmünkkel szemben előnyben kellene részesíteni a kód olvashatóságot, elvégre csapatmunkáról lenne szó.

Teljesen igazad van. Tényleg, de az összkép tekintetében az poligon létrehozó osztály refaktorálása felesleges erőfeszítés volt. A feladatot a @SzFMV2020-Tavasz/team-a2 kapta eredetileg az első sprintjében. Ezt tudták összehozni. Két komoly probléma volt vele.

1. Amit írtál és kifogásolsz.
2. Az egész elvi hibás. A feladatuk ugyanis (út elemet példának használva), hogy a sávokat jelöljék ki. Ezt egy útelem esetében 3 db. Line2D vonallal, GIS-es terminológiában LineString-gel lehet megvalósítani. Ők meg körbekeretezték az útelemeket egy-egy poligonnal. Erre nem lehet LKA-t építeni, így alapjaiban hibás az egész. Bekerült anno a masterbe, mert legalább a feltöltésre kerülnek a poligonok és a projekt azon szintjén tesztelési lehetőséget biztosított a jelenlétük (pl. a szenzorokhoz).
   • Tényleg érdekelne, hogy szerinted az én helyemben itt mi lett volna a helyes döntés. Visszatartom a PR-t, mert a kód nem teljesíti a normákat és két másik csapatot blokkolok hetekre, vagy amit tettem, hogy elfogadom egy iterációnak, amely majd remélhetőleg javításra kerül. Én akkor még nem tudtam kipróbálni, hogy mit tud a kód funkcionálisan, külön meg nem fejlesztettem rá tool-t, hogy megnézzem a poligonokat.
   • a múlt heti megbeszélésen véglegesítésre került @ArchiCat felvetése, hogy ő megcsinálja az összes poligont JSON fájlokba téve őket egy erre alkalmas eszközzel és lecseréli az általatok is kifogásolt kódot. Az ő megoldása mindkét problémát orvosolja egyszerre, a tiéd csak az elsőt, azt ami funkcionálisan kisebb jelentőségű.

Ez a PR nem fog (így) a masterbe kerülni, mert a poligonok másképp lettek megoldva, és az a megoldás jobb. Az enum-os refaktor teljesen valid clean code meg egyéb szempontból is, de nem kritikus.

Én nem is tudom, hogy utoljára mikor jelent meg valaki órán a Team3-ból. Mikor értesültem arról, hogy mikor mit szándékoztok csinálni és miért. Titeket mi blokkol és azt hogyan lehetne feloldani. Az egész PR egy „no-ticket refactor”, amihez nem tartozik task, senki nem kért és nem volt a @SzFMV2020-Tavasz/team-a3 feladata.

A team 4-nek pl. felvetése volt (múlt héten), hogy a hajtásláncból csak egy iteráció késéssel tudják elérni az autó koordinátáit. Persze most lehet mondani, hogy miért nem vettek fel erre taszkot hozzátok... mindenkinél lehet hibákat találni (nálam is rengeteget).

Ez a tárgy és ez a projekt 20, de nagyon minimum 16 aktív emberre lett tervezve, most van nagyon maximum 6. Nem működik az egész. Nincs kommunikáció. GH-on egyáltalán. Az egyetlen olyan dedikált időben, amikor biztosan meg lehetne beszélni dolgokat nem jelenik meg csak 2 ember (Team 4-ből). Tehát egy csapat van képviselve. Azt is mondhatnám, hogy egy csapat nézőpontja érvényesül, hiszen nincs ellenvélemény. Múlt héten kértek és kaptak új feladatot (#191) a 3. sprintre. Ez a PR összefog egy rakás munkát, amely olyan feladatot old meg ami nem volt feladat és nem vitte előre egyik sprinteteket is.

@ArchiCat azt mondta, hogy dolgoztok az NPC-ken egy branchen, mert látta ennek nyomát. Ez a PR kritikus volt ahhoz pl.? Mi kell ahhoz, hogy legyen 2-es sprintetek? Meg 3-as sprintetek?

Sajnálom, hogy a tárgy most már 1,5 éve kb. csak vegetál pár emberrel. Sajnos a neptunban nem tudom beállítani, hogy a git előfeltétel legyen, sem azt, hogy a java. Utóbbira a neptun meg a kar úgy tekint, hogy megvan a szakmai szigorlat, akkor megvan a java tárgy és teljesül. Hiába tudom, hogy ez nem igaz.

[aether-fox]

This pull request is no longer relevant.