Projekt do portfolio – code review

Odpowiedz Nowy wątek
2019-01-19 14:01
0

Hey, mógłbym prosić o code review mojego projektu do portfolio? (backend - Spring + frontend - React)

https://github.com/gjmOfficial/Centrala-schroniska

Chciałbym jeszcze dodać kilka rzeczy do tego projektu, ale już na tym etapie otrzymać jakiś feedback.

Pozdrawiam, gjm

edytowany 1x, ostatnio: furious programming, 2019-01-25 17:15

Pozostało 580 znaków

2019-01-28 17:49
2

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

Pozostało 580 znaków

2019-01-29 17:47
0
p_maciek napisał(a):

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

Przy każdej aktualizacji danych schroniska - dodanie/usunięcie zwierzaka następuje zsynchronizowanie danych w schronisku (wewnętrznego stanu beana Shelter)

Pozostało 580 znaków

2019-01-30 16:40
0

Ref :P

Pozostało 580 znaków

2019-01-31 13:05
1

Zamiast tego switch'a

https://github.com/gjmOfficia[...]helter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficia[...]dfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>
edytowany 1x, ostatnio: losowynick, 2019-01-31 13:19

Pozostało 580 znaków

2019-01-31 15:22
0
losowynick napisał(a):

Zamiast tego switch'a

https://github.com/gjmOfficia[...]helter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficia[...]dfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>

Mógłbym dopytać się o sposób usunięcia switcha?

Jeśli skorzystam z wzorca projektowego - Strategii, to powstanie metoda np. convertShelterReport(), która jako parametr będzie przyjmowała implementację interfejsu strategii np. ShelterReportConverter. Ale znów będę musiał użyć switcha albo jakiejś innej konstrukcji warunkowej, by określić czy przekazać implementację ShelterReportConverter dla PDFa czy dla CSVa... gdzie robię błąd myślowy?

Pozostało 580 znaków

2019-01-31 16:07
1

Nie znam Javy potraktuj to jako pseudo kod.

class ShelterReportConverter {
    List<RaportPolice> polices;

   //ctor
   ShelterReportConverter(List<RaportPolice> polices) {
        this.polices = polices;
   }

   public Report ToReport(string format) {
       return this.polices.filter(x => x != null).map(x => x.Convert(format)).firtst();
}

class PdfPolice implements ReportPolice {

    public Report Convert(string format) {
        if (format == "pdf")
        {
         //logic
         return report;
        }

        return null;
    }
}

Pozostało 580 znaków

2019-01-31 17:47
0
hiimRealgjm napisał(a):
losowynick napisał(a):

Zamiast tego switch'a

https://github.com/gjmOfficia[...]helter/ShelterController.java

Zrób strategie - politykę

Używasz dziwnych skrótów myślowych.

 bos = new ByteArrayOutputStream() // Bos schroniska. ?

Nie da się tego zrobić prościej w Javie na zasadzie jakiegoś mappera albo obiektu persistencji z adnotacjami?

https://github.com/gjmOfficia[...]dfShelterReportConverter.java

Nadużywasz wyjątków lepiej by było dać Either'a.

@Override
    public void registerWorker(Worker worker) {
        if(workerDao.findWorkerByName(worker.getName()) != null) {
            throw new WorkerAlreadyExistsException("Pracownik \"" + worker.getName() + "\" już jest zarejestrowany!");
        }

        workerDao.save(worker);
    }

Zabawne jest to że JPA wymusza na was stosowanie słowa repository tam gdzie jest ono nie potrzebne.

WorkerDao extends JpaRepository<Worker, Long>

Mógłbym dopytać się o sposób usunięcia switcha?

Jeśli skorzystam z wzorca projektowego - Strategii, to powstanie metoda np. convertShelterReport(), która jako parametr będzie przyjmowała implementację interfejsu strategii np. ShelterReportConverter. Ale znów będę musiał użyć switcha albo jakiejś innej konstrukcji warunkowej, by określić czy przekazać implementację ShelterReportConverter dla PDFa czy dla CSVa... gdzie robię błąd myślowy?

Tak, w jakimś miejscu taka logika musi być, możesz użyć do tego celu wzorca fabryki w ktorej zawrzesz logikę jaką implementację zwrócić.

Pokaż pozostałe 2 komentarze
W .net bynajmniej... - W2K 2019-01-31 19:40
Nie w .net tylko w dotnet. - losowynick 2019-01-31 19:50
Aby nie robić instrukcji warunkowych możesz też uzyć mapy w której wrzucisz konkretne implementacje pod odpowidające kody i wtedy uzycie baędzie bardzo proste : strategyMap.get(format); - p_maciek 2019-02-01 09:26
W sumie ta taki bardziej "pyłek" niż strategia. - losowynick 2019-02-01 14:23
Już zrobiłem fabrykę, możecie zerknąć :P - hiimRealgjm 2019-02-01 17:12

Pozostało 580 znaków

2019-01-31 17:51
1
hiimRealgjm napisał(a):
p_maciek napisał(a):

Mi się nie podoba koncepcja springowego beana "shelter" połączona z przechowywaniem stanu obiektu w środku. Nie masz synchronizacji pomiędzy operacjami modyfikującymi stan schroniska a pobieraniem raportu. Może to skutkować zwróceniem nieaktualego wyniku(z poprzedniego stanu).

Przy każdej aktualizacji danych schroniska - dodanie/usunięcie zwierzaka następuje zsynchronizowanie danych w schronisku (wewnętrznego stanu beana Shelter)

Chodziło mi o taki przypadek w którym ktoś się wstrzeli między opracją modyfikującą a aktualzująca stan i dostanie niepoprawny wynik ponieważ kontener nie będzie jeszcze odświeżony.

edytowany 1x, ostatnio: p_maciek, 2019-01-31 17:51

Pozostało 580 znaków

2019-01-31 19:27
W2K
0

Dlaczego commity są pisane po polsku ? Readme też nie powinno być po polsku.

Bo uważałem (nie wiem czy trafnie), że jeśli projekt do CV to tak powinno być. Jakie masz zdanie na ten temat? - hiimRealgjm 2019-01-31 19:29

Pozostało 580 znaków

2019-01-31 19:33
W2K
0

Bardzo nietrafnie uważasz. Prawdę mówiąc jako potencjalny pracodawca przeglądający Twój kod zanim jeszcze tam zajrzę od razu podejrzewam że coś jest nie tak z taką osobą. Kod pisze się po angielsku, i tak samo nazywa się commity i w tym języku pisze się readme. Co jeśli Twoja aplikacja będzie przekazana innemu zespołowi używającyemu innego języka ?

edytowany 1x, ostatnio: W2K, 2019-01-31 19:34
Ok, rozumiem. Ale aplikacja chyba może być po polsku (ad. komunikaty, UI)? - hiimRealgjm 2019-01-31 19:36
Nie używaj komentarzy do odpowiedzi. - W2K 2019-01-31 19:36

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

1 użytkowników online, w tym zalogowanych: 0, gości: 1, botów: 0