Code Review: Who Can Help Me

this time, taking on who can help me , i found a few really interesting things.

for one thing, we've got this little guy:

i was all ready to discover inewsrepository –<> newsrepository : repository<newsitem>, etc. what i found, instead was a real service:

to be frank, that's how it's supposed to be. there is a need to abstract something, and we write just enough code to make it work. i would argue with the implementation of this, however, because this particular approach to multi threading is wrong headed. we shouldn’t spin out a new thread pool work item just to execute the request when the fluenttwitter api already contains async version that can do quite well for us, but the overall concept is sound. and i was relieved not to find nested repositories in my first few steps there.

of course, i then discovered that this nice service has some friends from the left side of the blanket:

i think that i expressed my opinion about code like this:

a waste of keystrokes, and highly inefficient to boot. you don’t make queries by id in nhibernate, you use get or load instead.

overloading the infrastructure – after reviewing the code, i was surprised to see so much of it dedicated for caching:

what surprised me more is that in the entire application there were exactly two locations where a cache was used. in both cases, it led to the same service. implementing a much simpler solution in that service would have chopped quite a bit of code out of this project.

and then there was this:

luckily, this is dead code, but i was quite amused by this code, in a “shake your head in disbelief” fashion.

first, for a code that is obviously meant to be used in a multi threaded fashion, it is not thread safe. second, it is actually a memory leak waiting to happen, more than anything else. if you call that method, your items will never freed.

the next is a personal opinion, but i can’t help feeling that this is pretty heavy weight:

well, that is true whenever you are looking at an xsd, but in this case, we just need to expose two properties, and i think that it would have been perfectly fine to stick that into the <appsettings/> section. there are actually several places where similar approach has been tried, but i don’t see this of any value if you aren’t writing a library that might require special configuration. if you are writing an app, using the default modes is usually more than enough.

reading the controllers code wasn’t a real surprise. one thing that did bother me is the amount of mapping that is going on there, and how much of that i was simply unable to follow. for example:

which is then calling;

which then goes into additional custom implementations and convention based ones.

the reason that this is important is that this is a prime location for select n+1 issues, and indeed, we have several such occurrences of the problem just in this piece of code.