code-reviews 2021-01-22

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

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

what does the code look like currently?

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

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

I hope it up to standard

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

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

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

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

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

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

yep and then memoziated

so the next time it is faster for the user

but for a new user it is slow again

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)

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

only thing I have to figure out how to do so

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

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))

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

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

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

then I cannot pre-load things

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

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

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

now I miss you

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

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

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

I guess you can still sneak a prefetch in

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

sorry, you are going to fast for the clojure knowlegde

that's not clojure it's defroutes

where is the function inlne ?

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

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))

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

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

I have to make the pre-fetch function myself ?

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

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

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))

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

for me for now GN

and thanks for the ideas and the feedback

why is the future inside the ->

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

sorry. I thought you were saying that to me

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

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

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

time to sleep

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)

when wants to see th site

I will see your respons tomorrow when Im awake

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