What do you look for in a code review?

I have thought about this question a lot. I think the ultimate answer is "non-functional requirements": simplicity, maintainability, security, performance, test coverage (not the number; how many of the functional requirements are encoded into tests).

This comes from the point of code review: to make the software we are working on more useful.

There is a big list of non-functional requirements on Wikipedia; I will cover the ones I think are most important below. I'm biased toward maintainability because I've spent many years maintaining other people's code.

Simplicity

For me, this is the most important thing. Software dies when it becomes too complex. Development slows to a crawl when it is not possible to keep the important architectural components in your head. Changing anything becomes a process of trial-and-error which takes forever, and usually makes things even more complicated.

So, it is important to be constantly vigilant about simplicity. Sometimes that means saying no to a new requirement. Sometimes it means using a library instead of writing new code. Sometimes it means NOT using a library to avoid the complexity of having to manage that dependency. Often, none of the above are possible and the best you can do is refactor.

Simplicity applies at all levels. Requirements, architecture, module-level design, and individual lines of code. Even the size of an individual pull request. Keep them all as simple as possible. Jargon often used on this topic is "domain boundaries" or "high-cohesion/low-coupling" or "cyclomatic complexity" or "cognitive complexity".

Red flags for simplicity are:

1. singletons/globals/side-effect-heavy code
2. long, generic names
3. many lines of code in one function
4. unnecessary abstraction (also known as "overengineering")
5. "surprises" or "magic" in the code
6. Flag arguments (boolean parameters that switch behaviour)
7. Inheritance heirarchies
8. Mutexes

"Keep it simple" is my #1 thing for code reviews.

Test Coverage

When looking at test coverage, I will try to work out how many of the edge cases in the requirements for the change have been encoded into tests. For example, if the change is to send an email when an order is complete, there should be a happy-path test to make sure that whatever email interface is getting hit, and that the request has some of the strings the email is expected to contain. If the change is to process multi-leg orders, the tests should demonstrate how the obvious two orders get processed, but also that mixing a buy and sell leg is OK and that having three buy legs works, and maybe even that a leg with zero quantity or negative price is handled correctly.

Having tests like this makes the software maintainable. A new maintainer can come along and re-discover all the requirements that have been built into the program by reading the tests. There is no easier way for a programmer to learn the requirements (apart from having been there when the requirements were determined).

This allows new maintainers to understand the program -- and this allows them to keep their changes simple.

For a discussion of the other kind of tests ("don't test the glue"), see https://stackoverflow.com/q/12860657. Aiming for 95% test coverage (or whatever) almost always leads to writing useless tests that become a maintenance burden.

Readability

Going hand-in-hand with reading the tests, maintainers will spend a lot of time trying to figure out the code itself. There are a few things that help a lot here.

1. Comments that explain WHY. How, what, etc. I can get from reading the code. Hopefully, the imporant part for understanding -- the why -- comes from reading the tests; sometimes it doesn't.
2. Lots of descriptive names -- variables, functions, etc. Don't use generic words ("-Manager" is my pet hate), they don't help. Be specific.
3. Auto-formatters. All the big languages have this now. Stop wasting your time by not using them. Black, prettier, clang-format, go-fmt, whatever.

Teamwork

The best code review is one you do together. It's not worth trying to have difficult conversations through text when you can talk to someone directly.

If you're doing a code review and find something that's not a super-quick fix, it's almost always better to discuss it in person.

For everyone else's benefit, it is generous to record the outcome of the discussion in the review.

It seems the Ministry of Magic is interfering at Hogwarts

https://youtu.be/LBmHliQMY8w

What is CORS?

CORS is Cross-Origin Request Security.

You can set up CORS for your site by:

1. responding appropriately to OPTIONS requests (see the access-control- headers above), and
2. adding the access-control-allow-origin header to your normal HTTP method responses (GET, POST etc.) as well.

Why is CORS a thing?

In the beginning, javascript was only allowed to make requests to the same server as that bit of javascript came from. This was called the Same-Origin policy. Without the Same Origin Policy, sites would be able to make requests as each other: Mallory's website would be able to call Alice's servers, and the browser would add Alice's authentication cookie to the request.

However, that policy proved restrictive, so CORS was added to allow websites to permit requests from other origins.

How it works

1. Some javascript tries to send a request to a different domain than where that javascript came from

   // alice.com
   fetch('alice-api.com/add_user', {method: 'post'})
   

2. The browser does the Same-Origin check and enforces the CORS policy:
   1. The browser sends the "preflight" request. This is an request-method=OPTIONS request to the url (in this case, alice-api.com/add_user):

          Host: alice-api.com
          User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
          Access-Control-Request-Method: POST
          Access-Control-Request-Headers: content-type
          Referer: https://alice.com
          Origin: https://alice.com
          Connection: keep-alive
      

   2. The different domain (in this case, alice-api.com) responds:

          HTTP/2.0 200 OK
          date: Tue, 06 Aug 2019 02:26:03 GMT
          access-control-allow-origin: https://alice.com
          access-control-allow-headers: Authorization,Content-Type,Content-Disposition
          access-control-allow-methods: OPTIONS,HEAD,GET,POST,PUT,PATCH,DELETE
          access-control-allow-credentials: true
          access-control-max-age: 7200
      

3. Now that CORS has been checked, the browser does the real request to the different domain: request-method=POST to alice-api.com/add_user. The response generated by alice-api.com must ALSO contain the header:

       access-control-allow-origin: https://alice.com
   

   or the browser will not accept it.

Pairing notes from session by Michelle Gleeson

Every time: Starting a session

• Can you both see the monitor? Get comfortable.
• how long will you be pairing for, when will the breaks be (every 45 minutes)
• when swap? (every ten minutes, every green test, other little goals?)
• talk through the business logic / user story

Every time: after a session:

• what was hard about the session
• what were the unexpected benefits?
• how can we make it better next time?

Key takeaway:

• most benefits realised when you combine pairing, clean code and test-driven-development.

Why pair?

• whole team accountability
• higher standards
• can go on holiday (no key-man dependency, no bus-factor, no handover)
• continuous handover
• continuous code review
• build strong team relationships (how long does this take?)
• increases inclusion and authenticity
  • story about girl who hated her job until that team started pairing
  • opportunities for juniors to work on critical items
  • everyone learns lots of tips and tricks
• deliberate action: challenge your own thinking
  • story about submarine verbalising routine possibly-dangerous actions

How to pair?

• no phones
• one monitor
• same thought process
• low level hum of quiet discussion
• if on laptop, plug in a keyboard (no hogging screen)
• mirror displays
• no slack

Quickstart to get a team used to pairing

• identify why (measurable goals, e.g. 150% improved cycle time)
• agree to a two week experiment
• work through discomfort
• be courageous
• review regularly
  • what's hard
  • what are the unexpected benefits
  • how can we make it better
• block out time in calendar and link slack status to it so you don't get interrupted

tips and tricks:

• offsite pairing might help
• team charter to make sure values aligned
• development style: use a linter!

advanced:

• mob once a week. Several developers sitting around a shared TV, passing around the keyboard.
• use a timer to swap
• track who you pair with to make sure you get everyone. Make a chart.
• pairing cycle: make a test pass, refactor, write next (failing) test, pass over the keyboard
• everyone knows what's going on, manager can ask anyone
• team size 4-6 (bigger gets inefficient)

google clean code by robert martin
17 november code retreat. kata. pair with someone for 45 minutes then delete your code. dojo. new challenge for each pair.

How to avoid writing unit tests

1. Use the type system -- compile errors are better than test failures
2. Use libraries instead of writing your own code (and having to test it yourself as well).
3. Say no to unnecessary features. Be ruthless. Every new feature means more testing.
4. Test all the glue with a single integration test. Testing glue with unit tests is a waste of time:
1. glue unit tests only ever fail when you're refactoring, so they are all maintenance and no protection
2. they are not useful as documentation (there's no point documenting glue)
3. This does NOT apply to unit tests for business logic (as opposed to glue)
5. Write simpler code.

More thoughts on Go

following The Flaws Of Go
- go is good for web servers
- garbage collector means it is not a high-performance language (Rust better here)
- `panic` is often abused as an exception mechanism
- no LLVM because it is too slow (Rust bad here)
  - this means go doesn't target as many architectures
  - fast compiles are a major feature. compile all your dependencies every time removes a lot of headaches:
    - no more writing or learning a tool that manages dependency binaries
    - no more storing dependency binaries for every architecture you might need to support
    - no more binary incompatibilities
- no `const` or immutable structs is very bad, encourages poor architectural choices (Rust better here)
- simple syntax is good (Rust very bad here)

Don't test the glue

Software code is basically made up of two things: business logic and glue. There are other names for it -- boilerplate, wiring -- but it is basically just code that has to be there to let the business logic do its job.

You don't want to test the glue. It is incidental code. Maintaining those tests are going to be painful and pointless.

You do want to test the business logic thoroughly. Your tests are documentation, and allow you to refactor and port to other systems.

You don't need glue tests when refactoring because refactoring is about changing the glue. Having glue tests means more work when refactoring, not less.

You don't need glue tests when documenting because the important thing about documentation is the "why" -- why the business logic does what it does, who decided it should be that way, which customer asked for it, and so on. Glue has no why, it is just incidental code. Obviously there are exceptions to this -- a complicated threading model may be the best option and should be documented and tested, for example.

You don't need glue tests when porting to other systems because they will have different glue.

Don't test the glue, except in a very small number of integration tests.

PS. "Full coverage" when doing coverage reports of which lines of code got executed in tests is not a good goal, because it means you have to test all the glue. Read a coverage report by checking that lines containing business logic are tested and ignore untested glue.

javascript object key iteration order

I hate javascript.

Object.keys({a:true, c:true, b:true, 1:true, 3:true, 2:true})
> ["1", "2", "3", "a", "c", "b"]

Analyzing an unknown project's architecture quickly

I have been doing job interviews at work. One of the things our process involves is the "toy robot test" -- code up a solution to a problem where you need to make choices between common tradeoffs.

When analyzing the architectures to decide if I want to recommend this person to be hired as my boss, I found that the easiest way to get a quick understanding of the code was to draw what I call a "concept usage graph".

The process is as follows. I read each file, and every time a concept uses another concept, I add a link in a .dot file. Then I render the dot file and I have the graph.

What is a concept?

A concept can be a function, a class, a whole file, or even a module. If there is a one-to-one relationship between two things (for example, a file has exactly one method), I only mention the top-level one.

How do you add a concept to your graph?

Here is an example. For the code in toyrobot (which I am actually not happy with; the point of the test is the discussion, not the code produced), there is a Model struct:

type Model struct {

    *Robot

    Table

}

So in the dotfile, we would add links from Model to Robot and Table:

```graphviz

digraph {

app -> { model commands }

model -> { model_robot model_table }

Here is another example. We have a Report command. When executed, that command reads or writes from the members of a Robot.

func (c Report) Execute(state *model.Model) {

    r := state.Robot

    if r == nil {

        return

    }

    log.Printf("Robot at %d %d %s\n", r.PositionX, r.PositionY, r.Current.ToString())

}

So we add a link from the Report command to the Robot:

command_report -> { model_robot }

This way, the diagram shows all the concepts you need to understand to be able to read a particular file. It also shows all the other concepts that use whatever is defined in that file.

That way on my second pass through the source, I can work my way up the dependency tree, and always understand all the concepts required to read a file.

What does a typical graph look like?

The graph as a whole also gives a good overview of the program. Here is my final graph:


The things I don't like about this diagram (and therefore architecture) are:
- there are too many links! Perhaps there should be an abstraction for the command to use the model?
- There are links from top to almost the bottom. Perhaps app should not depend directly on model? It seems like the business logic (the commands) are not well partitioned from the domain model (model_robot and model_table)?
- there is no obvious separation between glue code and business logic

toyrobot in golang

Wrote toyrobot in golang. Thoughts:

- Golang is kind of "dumb". It has a strong emphasis on simplicity. I found myself writing very simple code. However, it is still reasonably concise.
- Not having to worry about memory management felt weird in such a low-level language. Also, since there are still pointers, the only protection from null pointer dereferences is at runtime. I had several null pointer dereference bugs during my development. Fortunately they were all found by small and simple tests, so they weren't too hard to find and fix. In a large program that would suck.
- Getting the tools running in VSCode on linux sucked. I had golang 1.10 installed somehow, but wanted to use 1.12 package features. Trying to upgrade broke my vscode -- I installed it with the snap. Setting up GOPATH and stuff wasn't fun. VSCode still doesn't work properly, doesn't highlight syntax errors or test failures.
- Writing tests was pretty straightforward.
- The effectively instant build time is definitely go's best feature.
- I suspect I would have difficulty writing efficient go code. I haven't learned the complex rules for when a struct gets copied when passed by value or when something is allocated on the heap instead of the stack. Also, garbage collection means no hard-real-time applications; GC pauses mean you can't use it for financial trading, hardware interfaces, or anything else where being a few milliseconds late is a failure. I'm having trouble thinking of other situations where that might be a problem though.
- The error checking boilerplate is pretty annoying
- I didn't use concurrency or channels, but I can see that it would be pretty useful for any kind of server app that talks/listens to other programs
- I really liked the single-binary-that-contains-everything concept. No build folder!
- I found the dependency management pretty straightforward
- Golang.org / stackoverflow was pretty good for looking up how to do things like reading from a file or parsing strings.
- Maps and slices syntax is a bit awkward
- I LOVED the everything-is-plain-old-data-with-extension-methods. I think that design decision alone strongly guides programmers toward more maintainable (less tangled) architectures.
- Packaging system very solid. My code is available almost accidentally as a library because I posted it on github. The only thing you need to do to use it is to add `import "github.com/jnnnnn/toyrobotgolang"` at the top of the source files that need it -- no `package.json` or `conanfile.py` or anything else. Exporting things that start with a capital letter is neat too.

I would recommend golang for the purpose it was designed for, server-side application-level software. Which is probably most software these days. It's not going to work for native GUIs or really high performance stuff or hardware interfaces.

Devlogs 2

I was stressed on Wednesday this week, overwhelmed with keeping track of regressions caused by a change to a React component. However, I discovered a very useful way of dealing with it -- create a whole Trello board just for the problem. Here is a screenshot of the trello board once I'd fixed all the problems.



There are 11 things that gradually got added to the Problems list and got moved off it once I'd dealt with them.

I could also have written these things down on a notepad or post-its (or in my devlog) as I noticed them. Both those options would have worked pretty well too.

Without Trello

I would start the refactoring, do a manual test, and notice that some things were broken. I tried to remember every problem (there were 11 separate problems in the end) and quickly got overwhelmed. I hate missing things in work so this was quite upsetting, and I found myself looking for distractions instead of focusing on the work.

With Trello

When doing manual testing, whenever I noticed a problem, I just added a card to the new Problems list. And then promptly and happily ignored it and forgot about it. Once I'd finished the thing I was working on, I could come back to the Problems list to see what I needed to fix.

This also allowed me to prioritize work. Instead of having the "native scrollbars option" idea and then immediately rabbit-holing into the problem, I just added it to the list. When I came back to it, I decided that it wasn't worth doing -- although I didn't have to immediately discard the idea, keeping it around to discuss next time someone else on my team had time to talk.

Why Humour

Large haskell program architecture

I've been doing job interviews lately and reviewing applicant's code has got me interested in quickly understanding abstractions and architectures in a novel codebase. My first step is now to draw a dependency graph -- read the code, and every time a module/function/file mentions a name, add a node for that name and an edge to the graph. This quickly builds up a picture of how the parts of the codebase are tied together. Easily-understood architectures have few edges. Bad architectures have edges from every node to every other node.

I've started applying this approach to larger programs I am considering adopting as well. For example, here is the dependency diagram for Pandoc, generated with

find pandoc/src -name '*.hs' | xargs stack exec graphmod -- -q -p | tred > graph.dot
dot graph.dot -Tsvg -ograph.svg

"I don't know how to fix leaks in museums" is my new answer whenever I am unable to help someone when they ask for help.

Backpressure

Our software uses sequence-based recovery, just like TCP. Each write to the database records the sequence number, and when we start recovery we ask the other end of the connection for the next sequence number that wasn't written to the database.
However our system processes messages in several stages between several different applications. The first stage can process messages far faster than the part that writes to the database.
If there are lots of messages cued up on the other end, the database part can get overwhelmed. So if we go down for a few minutes at a busy time, we can never catch up.  We try to read in too many messages, the database writes a few then crashes, and then we start again.
The standard solution to this problem is back pressure. If the database part refuses to accept too many messages, then the reader part has to wait for the database to be ready before sending more. This means that we only request messages from the other end when we have space in the queues.
Also the database part get slower if we send it too many messages at once. Its memory fills up, the processor and eventually disk caches get less efficient, and after a while it runs out of memory completely.
So back pressure actually makes the system considerably faster.
And also means it doesn't crash under load.

peak complexity

One architecture

Here is the concept usage graph of a good architecture. Note that there is a clear hierarchy.

Another architecture

Here is the concept usage graph of a more spaghetti architecture. Note in particular the MultiStepAction -- I find this kind of abstraction (a group of other abstract actions) very common in spaghetti architectures.

I want a pony

Life goals

1. Build a passion. Make a good living from it. Evaluate your personality's strengths and weaknesses, choose something you are suited to, and work at it until you love it.
2. Share an emotional connection with people who share one with you.
3. Spend time with people you respect and admire.

The meaning of life

If you're looking for the meaning of life, the answer you need is in a book called "Running on Empty" by Jonice Webb.

I spent sixteen years investigating physics and philosophy and all kinds of bullshit but it's all laid out in there.

Good luck!

keeping a mental model of threading when writing low-level multithreaded code

You have a tree of objects in your program, e.g.

But there is also a tree of threads!

It is important to always keep BOTH mental models in your head when writing C++ code. Just like you need to track which object owns which other object (for freeing memory and avoiding memory leaks), you ened to track which thread is executing each line of code (for avoiding race conditions). This is a very common error in c++ programming, and several patterns have sprung up to avoid having to think about it. Debugging race conditions is very difficult because they are non-deterministic (they may not occur on every execution of the program, making them hard to test repeatably).

Patterns for avoiding thinking about threading in c++ include

• a god object with locks (the worst option, not really multi-threaded)
• message passing (erlang, golang channels, "event-driven" frameworks)
• immutable objects (functional languages)