vakabus/reminderbot

reminderbot's Issues

Code review notes [in Czech]

1. README.md example input:

"please remind me by mail about mom's birthsday present at 30th May in the morning" nefunguje.

2. Neco k Dockerfilu

(jak uz jsi asi poznamenal v README.md) to co v nem delas opravdu neni best practice. Idea Docker image se nememi a mas porad stejne chovani; doporucuji si neco precist o multistage docker files. V prvni pomocne image s JDK a GRADLE si zbuildis (uber) JARko a to to ve finalni image s JRE jen spustis.

Tez iteraticni zpousteni pomoci bash scriptu ma mirne mouchy (nespousti se presne po 5ti minutach ale po 5 minutach + runtime programu); mozna by bylo stalo pouvazovat o nejake nejake java cron like knihovne.

3. Timezone

Kdyz jsem to skousel spustit tak jsem si nabehnul na time-zonu protoze v dockeru se pouzil UTC (cili casy byli o 2 hodiny posunute) mozna by mohl mit uzivatel specifikovanou vlastni timezonu v konfiguraci?

4. Ke kodu, celkove se mi libil.

a) neni uplne nejstastnejsi, ze logiku pro parsovaci textoveho vstupu nemas na jednom miste v jedne tride. Vetsinu parsujes v https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/Parser.java#L28 ale komu se nastavuje pripominka a jak se ma notifikovat se resi v https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/IdentityManager.java#L57-L84

b) Tento konkretni if: https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/IdentityManager.java#L57 se mi nelibil protoze vede na duplikaci kodu. Mimo jine branch nejsou ekvivalentni ( "me by email" to podle mne neda). Kod by si zaslouzil zrefaktorovat.

c) Visibility: v podstate ignoruje nastavovani visibility fieldu (nekde to za tebe resi anotace @value) ale treba https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/endpoints/email/EmailMessage.java#L15-L16 a https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/IdentityManager.java#L17-L18 https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/IdentityManager.java#L46 ale i jinde pouzivas defaultni visibilitu coz urcite neni dobre. Spis bych ocekaval private pripadne protected.

d) https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/endpoints/email/EmailEndpoint.java#L35 myslim ze obvykle se pouziva starttls pro non SSL conections v pripade SSL conections asi neni potreba. Docela me prekvapilo ze to fungovalo se seznamem a SSL smtp connections. Melo by to asi byt soucasti configurace jestli se ma pouzit nebo ne.

e) https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/endpoints/email/EmailEndpoint.java#L151 zvazil bych pridani "Re: " do subjectu odpovedi

f) https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/endpoints/email/EmailMessage.java#L42 Nejspis je jednoduzsi a efektivnejsi najit prvni vyskyt \n and pouzit susbstring. (plain old java :-) nez pouzit streamy.

g) https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/storage/User.java#L23 Zde bych ocekaval jako typ Interface, tedy Map misto HashMap

h) https://github.com/vakabus/reminderbot/blob/master/src/main/java/cz/vakabus/reminderbot/storage/User.java#L27 Tento radek, nejspis muze vyhodit vyjikmu, NPE ( v pripade ze bych v jsonu mela polozku pole null, ale to jsem netestoval) a pokud byde pole imalu prazdne pak muzes dostat vyjimku z iteratoru. Navic, pokud by ten Set (jako value mapy) byl HashSet, tak se ti emailova adresa mohla menit mezi behy programu, coz asi neni nejlepsi vlastnost. Myslim, ze by diky tomu mohli remindery chodit na ruzne emailove adresy. Nejjednoduzsim resenim asi bude mit specialni polozku v konfiguraci primary_email, jako email kam se maji posilat emaily s notifikaceme.

5. Storage

Delsi dobu jsem premyslel jestli je rozumne si pamatovat remendery tak, ze je mas jako neprectene zpravy v mailboxu (misto toho aby se to ukladalo po naparsovani v nejakem konfiguraku),
ale nakonec si myslim, ze to pro email neni spatna volba. Ale asi by jsi narazil, kdyby jsi delal Slack bota, tam by asi tento "trick" nefungoval.