

Architecture vs. Implementation Code Reviews - tomyedwab
http://www.arguingwithalgorithms.com/posts/14-12-02-architecture-reviews

======
darkFunction
I don't like the idea having long-running branches and especially TODO
comments in code before a review. I would argue that it is better to discuss
large architectural changes as a team, and split the task into smaller, more
manageable tickets and branches which can be reviewed and/or QA'd
individually. I think this is _key_ to maintaining clean and reliable.

I think the approach outlined is a bit upside down, and significant API
changes should be agreed on first. Not necessarily documented or thought about
in detail, but the implementer should walk away from the preliminary
discussion with a good idea of the agreed structure in order to be able to
flesh it out further.

This means the architectural changes on a high level are understood and agreed
on by the some or all of team first and the implementation is what actually
needs to be reviewed afterwards.

Thinking about a solution, pushing a draft to code review with your thoughts
explaining the changes typed out in full, responding to comments on the review
two days later, and repeating the process all over again- it just seems like
if you need this sort of feedback it would be way more efficient to schedule
an informal exchange in front of a whiteboard with one or more team members.

~~~
tomyedwab
Re: the first point. I agree that large changes should always be broken down
into individually testable pieces, however I don't agree that they can always
be broken down into individually _shippable_ pieces. This is definitely true
of large refactor projects, where a simple change (like upgrading a third
party library) will unavoidably trigger thousands of changes throughout the
codebase. I find it easier as a reviewer to look through a bunch of TODO
comments that outline what needs to be done, and then each subsequent review
addresses a manageable amount of the TODOs. I really see no other way since we
can't ship two copies of, say, jQuery to customers.

I totally agree with you that the architectural changes should be agreed upon
in advance, with direct conversations happening and whiteboards if necessary.
I tried to make it clear in the post that architecture _design_ should happen
before any coding begins, but the gap I noticed was that even with full
agreement on a schema or even down to the function level, there are still
important details to work out before writing logic and UI on top of it, and
that requires special attention from reviewers.

Just to play devil's advocate though, if you avoid face-to-face design
sessions and do everything through code reviews, you will be forced to explain
your thinking in text, and the whole conversation will remain as a record for
future developers getting familiar with the code who may be confused about why
things were done a particular way. The code review paper trail is immensely
helpful to understanding a large codebase - reading through sparse meeting
notes is never as good.

------
alphadevx
Very nice article. To be honest, I always review code from a architectural
perspective, rather than just reviewing code quality, as getting that stuff
wrong will hurt a project more so.

~~~
collyw
I have been looking for jobs recently. No one seems interested in testing my
architectural decisions.

Instead I get silly coding trivial pursuit questions, or built an application
to do this in 2 hours. Neither of these approaches seems to test what I am
good at after 11 years of software engineering - coming up with an
architecture that makes the correct trade offs (rushing a new project for a 2
hour deadline is probably the worst way for me to write code).

~~~
alphadevx
Completely agree. When I hire engineers, I get them to write an app at home
for a week and then send it in. I don't set an arbitrary deadline, just send
it when you think it's ready. I'm then looking at:

1\. Code quality (mainly with a view to maintainability)

2\. Unit test coverage (I don't even asked them to do that, the good ones know
to provide them).

3\. How the app uses dependencies (APIs, databases, caches), and if they
support dependency injection.

4\. Can the app scale up and down (i.e. can I scale horizontally by just
adding another instance?).

5\. How are they using 3rd party libs?

6\. How well documented is the solution?

7\. How easy is it to build and deploy?

8\. How well structured is their database (PKs, FKs, indexes, no excessives
joins etc.)?

9\. Is their a clean separation of the main modules in the app? E.g. MVC,
having a REST API for the data and de-coupled clients etc.

I could go on. At a senior level, I just assume an engineer can write nice
code in whatever language, but also has a keen understanding of software
architecture with a view to how their code impacts on scaling and performance.

As a candidate, if all I get asked at interviews is low-level algorithms, I
move on.

~~~
collyw
That's quite a thorough list, which I appreciate, but on the other hand its a
lot to do for an interview.

~~~
alphadevx
That's why I give them at least a week to do the code submission at home. It
is too much to cover in just a few hours in an interview.

~~~
collyw
Its also quite a lot t expect. After a couple of half days worth, with no
result at the end, I am getting kind of pissed off with them. Do you pay them
for this?

~~~
alphadevx
No we don't pay, and you're right it is a lot to expect. I've been on the
other side myself plenty of times so I know how that feels: nothing more
frustrating than having your time wasted. But on the flip side, I now have to
waste a lot of time screening, interviewing, and doing code reviews with
candidates that did not work out. Hiring sucks from both sides, I wish you
luck.

