code-reviews

roelof 2021-01-22T18:28:56.001Z

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

roelof 2021-01-22T21:19:09.001400Z

@noisesmith

roelof 2021-01-23T10:08:57.012300Z

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

2021-01-23T19:31:27.012500Z

what does the code look like currently?

roelof 2021-01-23T19:55:27.012700Z

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

roelof 2021-01-23T19:55:52.012900Z

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

roelof 2021-01-23T19:56:30.013100Z

see repo: https://github.com/RoelofWobben/paintings

roelof 2021-01-23T19:56:40.013400Z

I hope it up to standard

roelof 2021-01-23T19:58:24.013700Z

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

2021-01-23T20:07:28.013900Z

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

roelof 2021-01-23T20:25:16.014100Z

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

2021-01-22T21:34:41.001500Z

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

roelof 2021-01-22T21:54:41.001700Z

nice to hear

roelof 2021-01-22T21:55:15.001900Z

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

2021-01-22T21:56:21.002100Z

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

roelof 2021-01-22T21:56:40.002300Z

yep and then memoziated

roelof 2021-01-22T21:56:57.002500Z

so the next time it is faster for the user

roelof 2021-01-22T21:57:09.002700Z

but for a new user it is slow again

2021-01-22T21:57:55.002900Z

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)

roelof 2021-01-22T21:58:39.003100Z

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

roelof 2021-01-22T21:58:54.003300Z

only thing I have to figure out how to do so

roelof 2021-01-22T21:59:10.003500Z

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

2021-01-22T22:00:35.003700Z

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

2021-01-22T22:01:04.003900Z

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

roelof 2021-01-22T22:02:23.004100Z

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

roelof 2021-01-22T22:02:55.004300Z

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

roelof 2021-01-22T22:03:11.004500Z

then I cannot pre-load things

2021-01-22T22:04:26.004700Z

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

2021-01-22T22:05:02.004900Z

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

2021-01-22T22:05:26.005100Z

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

roelof 2021-01-22T22:05:54.005300Z

now I miss you

roelof 2021-01-22T22:06:43.005500Z

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

2021-01-22T22:06:52.005700Z

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

2021-01-22T22:07:06.005900Z

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

2021-01-22T22:07:22.006100Z

I guess you can still sneak a prefetch in

2021-01-22T22:07:53.006300Z

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

roelof 2021-01-22T22:08:03.006500Z

sorry, you are going to fast for the clojure knowlegde

2021-01-22T22:08:15.006700Z

that's not clojure it's defroutes

roelof 2021-01-22T22:08:15.006900Z

where is the function inlne ?

2021-01-22T22:08:47.007100Z

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

2021-01-22T22:09:40.007300Z

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

roelof 2021-01-22T22:10:08.007500Z

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

2021-01-22T22:11:13.007800Z

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

roelof 2021-01-22T22:11:35.008100Z

oke

roelof 2021-01-22T22:11:58.008300Z

I have to make the pre-fetch function myself ?

2021-01-22T22:12:50.008500Z

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

roelof 2021-01-22T22:13:04.008700Z

oke

roelof 2021-01-22T22:14:16.008900Z

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

roelof 2021-01-22T22:15:07.009100Z

oke

roelof 2021-01-22T22:16:20.009300Z

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

roelof 2021-01-22T22:16:54.009500Z

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

roelof 2021-01-22T22:17:52.009700Z

for me for now GN

roelof 2021-01-22T22:18:12.009900Z

and thanks for the ideas and the feedback

2021-01-22T22:21:03.010100Z

why is the future inside the ->

2021-01-22T22:21:30.010300Z

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

roelof 2021-01-22T22:21:35.010500Z

sorry. I thought you were saying that to me

2021-01-22T22:22:55.010700Z

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

2021-01-22T22:23:07.010900Z

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

2021-01-22T22:24:02.011100Z

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

roelof 2021-01-22T22:25:29.011300Z

time to sleep

roelof 2021-01-22T22:25:41.011500Z

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)

roelof 2021-01-22T22:26:00.011700Z

when wants to see th site

roelof 2021-01-22T22:26:50.011900Z

I will see your respons tomorrow when Im awake

roelof 2021-01-22T22:27:17.012100Z

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