[OPJJ] Recenzija DZ2
MU

Marko Čupić <Marko.Cupic@fer.hr>
Fri 3/17/2017 5:10 PM
To:
Damjan Vučina;
Bok Damjane!

Kad sam već danas napravio recenziju Vaše zadaće, evo je mailom u nastavku.

Srdačan pozdrav,

MČ

ArrayIndexedCollection:
* remove(-1) očekivao bih da baci IndexOutOfBoundsException; baca
IllegalArgumentException, a u javadocu se nigdje ne spominje koju će
iznimku metoda baciti što je sa stanovišta korisnika te metode loše.
Isti komentar ako je indeks prevelik te ako se zove insert(..., p) gdje
je p nevaljao.
* insert(x,p) uopće ne ažurira size! Isto tako, ne provjerava što se
događa s kapacitetom pa se raspadne kad je kapacitet premali da bi se
element umetnuo. To ste kroz testove morali detektirati!
* konstruktor ArrayIndexedCollection(Collection collection, int
initialCapacity) uopće ne ažurira size nakon kopiranja! Kolekcija tvrdi
da je prazna!
* javni setSize(...)??? Jao, jao, jao! Pa sada bilo tko izvana može
postaviti to na bilo kakvu vrijednost (pozitivnu, negativnu, veću čak i
od trenutnog kapaciteta polja)! Ne, ne i ne. Ne to raditi. Korisnik
smije imati samo metodu za dohvat veličine - vaša implementacija mora
time upravljati.
* kapacitet kolekcije je interni detalj, korisnik ga ne smije vidjeti a
posebno ga ne smije moći mijenjati kroz javni setter koji uz to ne
održava nikakvu vezu između tog broja i stvarnog kapaciteta alociranog
polja!
* metoda getElements() a posebno u izvedbi koja vraća referencu na
interno polje NE SMIJE postojati (jednako kao niti setElements).
Zamislite da imate zločestog korisnika koji kaže: getElements() i potom
sve reference u tom polju postavi na null! Koji je efekt? Vaša kolekcija
odjednom sadrži null elemente za koje tvrdi da ne čini!

     public void add(Object value) {
         if (value == null) {
             throw new IllegalArgumentException("Cannot add null value
to the collection.");
         } else if (capacity == size) {
             capacity *= 2;
             elements = Arrays.copyOf(elements, capacity);
         }
         elements[size] = value;
         size++;
     }

==> pričali smo o tome na prošlom satu:

     public void add(Object value) {
         if (value == null) {
             throw new IllegalArgumentException("Cannot add null value
to the collection.");
         }
         if (capacity == size) {
             int newCapacity = capacity * 2;
             elements = Arrays.copyOf(elements, newCapacity);
             capacity = newCapacity;
         }
         elements[size] = value;
         size++;
     }
  Obratite pažnju da je potrebno kapacitet ažurirati tek ako ste uspjeli
realocirati polje.

* Indentacija koda (javadoc komentara) nad nekim metodama se razletila.
* Duplicirate kod: contains se može trivijalno izvesti preko indexOf;
indexOf bi trebao provjeriti je li vrijednost null i odmah vratiti -1
(znate da takva ne može postojati u kolekciji).
* metoda addAll uopće ne radi svoj posao: ona u trenutnu kolekciju treba
nadodati sve iz predane kolekcije; ne izgubiti svoj vlastiti sadržaj i
postati kopija predane!
* ArrayIndexedCollection(Collection collection, int initialCapacity),
ArrayIndexedCollection(Collection collection): što ako je collection==null?

Collection:
* Želimo jasan i jednostavan kod. Umjesto:
     public boolean isEmpty() {
         if (this.size() > 0) {
             return false;
         } else {
             return true;
         }
     }
  pišite
     public boolean isEmpty() {
         return this.size() == 0;
     }

LinkedListIndexedCollection:
* ne treba u konstruktoru first = null; last = null; Alokator memorije
(new) će sve postaviti na inicijalne vrijednosti pri alokaciji objekta
* LinkedListIndexedCollection(Collection collection) - što ako je
collection==null?
* setSize(int size)!!! Ali ovdje je to totalno besmisleno!
* get/set First/Last !!!
* Bravo za metodu fetchIndexedElement(...) i njenu uporabu u drugim
metodama! Super ste to napravili i njome ste odlično pokušali riješiti
druge metode!
* Nažalost, insert(Object value, int position) nije dobro riješen -
primjerice, kada dodajete čvor na kraj postojeće liste - taj kod ne radi
dobro!
* zbog toga, primjerice, addAll ne radi, kao niti insert(obj)
* indexOf(null)  treba vratiti -1, ne iznimku
* remove(Object value): zašto lanac contains pa indexOf - ne radite li
istu stvar dva puta?
* kako dodavanje ne radi dobro, u jednom trenutku se i remove raspadne s
NullPointerException, te niz drugih metoda, da ih sada ne navodim jednu
po jednu.

ObjectStack
* NE SMIJE naslijediti ArrayIndexedCollection; niste dobro shvatili zadatak.

ComplexNumber
* getAngle() vraća negativne kuteve što ne smije prema opisu te metode

Damjane, ova zadaća je definitivno korak na gore u odnosu na prethodnu
zadaću. Tamo ste se još kako tako snašli u proceduralnom kodu, no ovo
ovdje je naprosto loše. S obzirom da, ajmo reći, imate rješenja svih
zadataka (iako Vam ObjectStack formalno ne bih trebao priznati kao
rješenje), dat ću Vam ocjenu 2. Međutim, trebate se ozbiljno uhvatiti
posla ako želite ostati na ovoj vještini.

Za početak, da biste ostali na vještini, popravite ovu domaću i dođite
do mene najkasnije sredinom sljedećeg tjedna pokazati mi tu verziju (na
vlastitom laptopu).