bejta/1dv450_labb3

1dv450_labb3's Issues

Peer review

Hej, här kommer min peer review på din labb 3 :)

Tyvärr får jag problem med CORS så jag har inte lyckats testa hela applikationen. Om du använt cloud9 till både API:et och din klientapplikation så har det kanske funkat iallafall, så var det för mig. Det finns ett smidigt gem som heter rack-cors som löser detta ganska smidigt.

Ett tips kan vara att deploya både serverapplikationen och klientapplikationen, då vet du säkert att det finns en version som går att testköra.

• I din kod finns det en del DRY. URL:en till API:et och headers finns med i varje request, trots att du har dem som konstanter i app.js.
• Din HTTP-hantering är lite osmidig: nu använder du $http i dina service-klasser men även i SearchController och CRUDController. Det borde bara finns i service-klasserna.
• Du injectar även $http i nästan alla service-klasser och det blir en hel del upprepning här. Det smidigaste hade kanske varit att ha en http-service som dina andra services använder. Flödet hade då blivit t.ex. PubController -> PubService -> HttpService -> API. Fördelen då är att du endast behöver en funktion i hela applikationen som gör GET-requests, en för POST, en för PUT och en för DELETE, och det blir mycket enklare att underhålla och göra ändringar osv.
• Du injectar en del grejer som inte används, t.ex. $q i CRUDPubService eller filterFilter i PubEditController
• Det finns också en del "boilerplate-kod" kvar från när du skapade projektet, t.ex. js/services.js som bara tar onödig plats och gör koden lite jobbigare att sätta sig in i.
• När man kör applikationen har jag bara sett två småsaker nu: 1. kartan går att zooma/dra i om man kör mobilläge, men inte desktopläge, det kanske går att lösa smidigt med nån inställning? 2. Även om man inte skriver i nånting i inloggningsformuläret så skickas en request till servern.

I övrigt så tycker jag din kod ser bra ut. Den är tydlig och lättläst. Det är inte så mycket kommentarer, men det tycker jag inte behövs, då det mest är standard angular-grejer. Vyfilerna ser bra ut, men det hade kanske varit bättre att inte ha input-fält i p-taggar, utan i div-taggar? :)

Säg till om du fixar CORS, så kan jag kika lite mer på resten av applikationen om du vill :)