Code Review Homework

Sep 17, 2012 at 3:42 PM

I don't think indirection is an issue. The code seems succinct and clear in the individual modules. While I tend to like things spelled out more in this kind of sample code used for introductory purposes, these modules are short enough to review and figure out what is being done. The ability see the whole thing at a glance helps tremendously.

However, there is nothing here that describes the relationship of the modules to each other. No comments in the code, for example, that explains what the filstrippresenter is in relation to the detail presenter, or how the detail page is actually using all the presenters.

Perhaps this would be more appropriately covered in documentation/article about the project, but at least something that helps relate the modules together should be in the code comments.

I'm not versed enough in the testing framework being used to comment on the testability, so I'll leave that to others.

Sep 19, 2012 at 12:46 PM

It is ok for beginners like me.

Sep 19, 2012 at 9:54 PM

code is prity much clear, specs also.

i'have a question:

FilmstripPresenter and FlipviewPresenter are both created in DetailPresenter. On the other hand ImageNavPresenter is created in detail.js and passed to DetailPresenter constructor. Why is that?

Sep 19, 2012 at 10:36 PM

I agree with rofite. The code is quite clear but how it is all linked together from start of the hub to the eventual initial display. Again, as rofite has suggested this maybe suited to an article or something on the codeplex site, but it is not immediately apparent that how items are linked together. Perhaps an initial basic step by step list just showing the sequence of events from start of app, to the hob module and what series of events happen to present the initial display. Obviously you can follow this through via debugging but it does remove ambiguity and means speed to understanding is considerably shorter.

Also, it is not apparent that you need to have images in your Pictures folder (among others). Again, you can find this out by stepping through the code but a one liner in the readme.txt suggesting you should have some images in your Pictures/Homegroup/etc... (std windows dir) directories for images to display would be helpful. To be more precise, download app code, load in VS 2012, hit F5 to run. If there are no images, as there wasn't in my case, it is not clear whether this is intended or maybe the code has a bug? Dont know until you debug. Again, one line of text would set expectations.

Overall, the code is well commented and layed out. My main gripe as already mentioned, is the flow or linkage of modules.

Sep 19, 2012 at 10:40 PM

Sorry, I forgot to mention one more thing. I really dislike downloading some code, to find out that I have to go to a few other places to download more code. Additionally when I compile I get errors, prompting immediate concern about the codebase. So having to download chai.css/mocha.js/mocha.css is annoying. I would prefer it come with the code or an additional build step to retrieve this if not present. I would suspect 40%-50% OF people wont even bother with the tests if they have errors in that project, irrespective of how easy it is to download and copy in for the tests.