Fork me on GitHub
#code-reviews
<
2021-01-22
>
roelof18:01:56

version 1 is ready. so please some more feedback https://github.com/RoelofWobben/paintings

noisesmith21:01:41

I like this, the separate namespace and refactoring for html generation helps a lot

roelof21:01:41

nice to hear

roelof21:01:15

the only thing that bugging me is that it takes some 4 seconds before a page is loaded

noisesmith21:01:21

is the remote resource being queried when you generate every response?

roelof21:01:40

yep and then memoziated

roelof21:01:57

so the next time it is faster for the user

roelof21:01:09

but for a new user it is slow again

noisesmith21:01:55

one alternative is to have a startup step that pre-caches (so instead of sitting idle and doing the work when the first request comes in, it can do it immediately)

roelof21:01:39

yep, I was just thinking of that step so I pre-load the next page

roelof21:01:54

only thing I have to figure out how to do so

roelof21:01:10

too less xp with clojure to know how to make that work

noisesmith22:01:35

for simple things where you just want something to run in the background and don't care if it fails or what it returns, you can just replace (f x) with (future (f x))

noisesmith22:01:04

so you could use a future to ensure the next page is pre-fetching in cache, when this page is asked for

roelof22:01:23

oke, so make a future of the code that ask the data from the external api

roelof22:01:55

I have to look then how to do when the user wants to see the first page

roelof22:01:11

then I cannot pre-load things

noisesmith22:01:26

unless I am misunderstanding you that's pretty simple - clojure functions have an implicit "do" block so you can add side-effecting code before the part where it returns something

noisesmith22:01:02

but in your case this might mean moving some logic out of the router itself into a function

noisesmith22:01:26

(the router syntax sugar might be making what the code actually does less clear)

roelof22:01:54

now I miss you

roelof22:01:43

the router only calls some functions and as far as I understand things does not have some logic

noisesmith22:01:52

(defroutes app
  (GET "/" request (let [page (extract-page-number request)]
                     (-> (memo-display-data page)
                         (display/generate-html page))))
  ...)

noisesmith22:01:06

this is weird because it's creating an implicit function inline

noisesmith22:01:22

I guess you can still sneak a prefetch in

noisesmith22:01:53

(defroutes app
  (GET "/" request (let [page (extract-page-number request)]
                     (future (pre-fetch page))
                     (-> (memo-display-data page)
                         (display/generate-html page))))
...)

roelof22:01:03

sorry, you are going to fast for the clojure knowlegde

noisesmith22:01:15

that's not clojure it's defroutes

roelof22:01:15

where is the function inlne ?

noisesmith22:01:47

everything after Get "/" is using a compojure dsl to implicitly create a function

noisesmith22:01:40

I'd normally have

(defn home-handler
  [request]
  (let [page (extract-page-number request)]
    (-> (memo-display-data page)
        (display/generate-html page))))

(defroutes app
  (GET "/" home-handler))

roelof22:01:08

oke, I thought this was the right way of doing things

noisesmith22:01:13

it's a style choice, but I think beyond a line or two it's better to explicitly use a function

roelof22:01:58

I have to make the pre-fetch function myself ?

noisesmith22:01:50

what you might want is just (future (memo-display-data (inc page)) I just hand't filled out that part yet, so I used an imaginary function name

roelof22:01:16

Thanks. I will sleep about it Right now this is im afraid more then I already know about clojure and it is late here

roelof22:01:20

so if I understand you well the handler has to look like this :

(defn home-handler
  [request]
  (let [page (extract-page-number request)]
    (-> (future(memo-display-data (inc page))
        (display/generate-html page))))

(defroutes app
  (GET "/" home-handler))

roelof22:01:54

Right now I wonder if it's displaying the right page

roelof22:01:52

for me for now GN

roelof22:01:12

and thanks for the ideas and the feedback

noisesmith22:01:03

why is the future inside the ->

noisesmith22:01:30

you don't care what it returns (or even if it succeeds) when you are doing a prefetch

roelof22:01:35

sorry. I thought you were saying that to me

noisesmith22:01:55

(defn home-handler
  [request]
  (let [page (extract-page-number request)]
    (future (memo-display-data (inc page))
    (-> (memo-display-data page)
        (display/generate-html page))))

noisesmith22:01:07

the future is being done for side effects, so it shouldn't be inside the chain

noisesmith22:01:02

if it fails, you can deal with that next time (it won't get memoized, you'll see the actual error), so you can just fire and forget to preload cache

roelof22:01:29

time to sleep

roelof22:01:41

but I see now this error : 2021-01-22 23:24:45.403:WARN:oejs.HttpChannel:qtp1199115300-24: / java.lang.NullPointerException: Response map is nil at ring.util.servlet$update_servlet_response.invokeStatic(servlet.clj:100) at ring.util.servlet$update_servlet_response.invoke(servlet.clj:91) at ring.util.servlet$update_servlet_response.invokeStatic(servlet.clj:95) at ring.util.servlet$update_servlet_response.invoke(servlet.clj:91) at ring.adapter.jetty$proxy_handler$fn__7287.invoke(jetty.clj:28) at ring.adapter.jetty.proxy$org.eclipse.jetty.server.handler.AbstractHandler$ff19274a.handle(Unknown Source) at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) at org.eclipse.jetty.server.Server.handle(Server.java:501) at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:383) at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:556) at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:375) at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:273) at http://org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) at http://org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) at http://org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:806) at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:938) at java.base/java.lang.Thread.run(Thread.java:834)

roelof22:01:00

when wants to see th site

roelof22:01:50

I will see your respons tomorrow when Im awake

roelof22:01:17

thanks a lot for the feedback and helping me with the pre-loading

roelof10:01:57

@noisesmith any idea why I know get a error message that there is a empty respons map

noisesmith19:01:27

what does the code look like currently?

roelof19:01:27

@U01JARXUA75 and I solved it. We found 2 small issues with my code

roelof19:01:52

and make together version 3 with now a good working pre-load

roelof19:01:40

I hope it up to standard

roelof19:01:24

next would be to make the layout in react so I can make the images clickable and I can show a modal with some more info about the paintings

noisesmith20:01:28

aha, so the templates all move from server side code into client side

roelof20:01:16

yep, but then I think I have to learn react from the beginning