Fork me on GitHub
#code-reviews
<
2016-12-13
>
agile_geek08:12:36

@roelofw I had a very quick look at code. One comment, I don't think wrapping the pmap calls in a future is necessary. It just adds complexity.

agile_geek08:12:38

The pmap will parallelise all the calls over a thread pool and handle the semi-laziness for you, blocking as required when it's realised by the merge.

agile_geek09:12:59

Couple of style things - it's common practice to not leave trailing close parens ) on a separate line but have them close off the expression on the same line e.g.

art-objects (-> response
                        :body
                        :levels
                        )
Would look like:
art-objects (-> response
                        :body
                        :levels)

agile_geek09:12:57

Except slack has messed up formatting as the :body and :levels should line up under response

agile_geek09:12:00

I might think about names of functions too. Maybe read-data-painting is now better named format-painting-data for example.

agile_geek09:12:17

But generally you've learned a lot! Well done.

roelofw13:12:10

Thanks for the feedback , @agile_geek

roelofw13:12:38

I think I have to do things otherwise. On the first page I only need the image of a painting

roelofw13:12:33

and on a second page that I still have to build. You will see the same image with a place where the details are schown.

roelofw13:12:16

So Im thinking to delete the do-all function and make the call to the api in the home routers file

roelofw13:12:58

Can I still use the pmap there so things are "downloaded" in parallel.

roelofw13:12:46

When you then click on a image , then the details are pulled out of the api with the read-data-painting

roelofw13:12:18

Because the map is not a variable , I cannot reuse it on the to build detail page

agile_geek13:12:13

You can use pmap to parallelise calls for different urls (i.e. different painting ids) whereever

agile_geek13:12:23

Not sure I understand the bit about the build detail page? If the idea is to render a list of paintings and then click on a link to render the detail of a specific painting, pass the key of the painting (plus any other variables) in the query sting of the href on the link. Then destructure the query params in the function that handles that route.

roelofw13:12:02

yes. in fact for the detail page we have already desctruced it with read-data-painting

agile_geek13:12:45

Yeah but once you've returned a response you server side data is gone. Unless you store it in a session. So if you render the link to the detail with the data you need in the query string you don't need to store session data

agile_geek13:12:45

Remember you may have thousands (hopefully) of users at same time so one global variable storing id's of paintings might work in this site but if everyone's view was different it wouldn't

agile_geek13:12:12

Try and make your server stateless.

roelofw13:12:03

yep, that why I thought of first only download the numbers and the images and shown them

roelofw13:12:41

When I user clicks on a image then there will be another call to the api which pulls out the details and shown it

agile_geek13:12:42

What happens if someone bookmarks the details page and then goes there as the first page after a server restart?

roelofw13:12:00

so nothing gets "stored" anywhere

roelofw13:12:32

a nice one. I have to think about that

agile_geek13:12:45

If you render the images with an anchor with an href that includes all the information you need (i.e. id's) then you can catch this info in the function dealing with the route, extract the id of the painting and do the detail api lookup for that painting before rendering the detail page. Nothing is stored server side.

roelofw13:12:51

oke, in fact the same as I schould do when someone wants to see a detail page

roelofw13:12:32

but first some more 4clojure and standford challenges

roelofw13:12:13

and find a better layout which also works on mobile and so on. As you can see im not a front-end developer or a designer , @agile_geek

agile_geek13:12:39

neither am i!

roelofw13:12:49

join the club. Problems I have with most of the templates I found is that all paintings/photo's have the same dimensions. With me there are some landscape and some not. And there are not of equal width and height