How to not get a job that sucks

As I’m writing this, we’re in lockdown due to COVID-19. Nobody knows for sure what the world of work would look like once we’re out of this.
But we already know that the economy is going to suffer, in a major way.
This, unfortunately, means that some of us will lose their jobs, and will be looking for a new one.
A friend of mine recently wrote a post about preparing for a coding interview. She recommends “Cracking the coding interview” to prepare for algorithmic questions.
My immediate thought was: “If I’m evaluated on theoretical material that’s completely irrelevant to the day-to-day of the role I’m interviewing for, I’d rethink my application”.
I won’t go here into trying to convince you why a whiteboarding interview is a red flag (search the web for ‘whiteboard interview’ for some interesting opinions).
But it did give me the idea to write about another, often neglected, aspect of job-hunting: Evaluating a potential employer.
In this post I’ll share my process of gathering and analyzing data during the interview process. This helps me evaluate whether a particular company is a good fit for me. I’ve developed it over the course of many years, and a few very avoidable employment disasters.
I hope you find it useful.

How can you assess if a company is a right fit for you?

At its core, it’s a simple system: Like a scientist, try to gather as much evidence as possible. then try to draw conclusions based on them.

On their own, many pieces of evidence may seem trivial, unimportant, or can be excused away. But, putting them all together, they tell a larger story.
For example –
An interviewer being late to the interview isn’t too bad – we all run a little late sometimes.
Not offering you a drink of water during your 3-hour session is annoying, but not a big deal.
Being unprepared, not having read your resume beforehand, is irritating, but forgivable.
But all together paint a picture. If you aggregate all the above data points, you could conclude that, most likely, this company is not respectful of candidates.
In that case, you can deduce that the company isn’t very respectful of its employees as well.

Evidence-gathering methods

1. Questions
Those are the most obvious route: Ask your prospective employer questions that aim to gain insight into the company. Asking the right question in the right way can reveal a lot about a company.

But asking too many questions will make me seem condescending / snobbish / difficult, and they won’t want to hire me!
Think of it this way – a good company would love the opportunity to show how good they are. They won’t shy away from questions, because they have good answers that help their sales pitch.
On the other hand, being afraid of questions is an indication of the opposite.
Avoiding a hire because of a candidate’s legitimate concerns is a huge red flag. If that’s the case – be glad that you won’t work for an organization that punishes people for asking questions.

2. Observations
Those are the things that you see and hear during your interactions with the employer. The more attention you pay, the more observations you make, the more evidence you’ll have to draw conclusions from. Observations can be about anything – the speed with which your email is answered, the phrasing of their reply, the chat you had with the receptionist while waiting for your interview, the size and shape of the office.. Any of the above can be a small piece of a puzzle that would give you a clearer picture about the company.
Making observations can be even more valuable than asking questions. Unlike answering questions, the company can’t control the narrative of your observations. Company representatives can give favorable answers, but they can’t control what you observe. Your observations are more likely to be unbiased, and more revealing.

Undoubtedly, you’ll discard some (or many) observations as insignificant or irrelevant. However, remember – you can piece together many small pieces to create a larger whole.

Not all signals will be as obvious as this

Deal breakers

Before we go into subtleties and clues, it’s worth thinking about what your line in the sand is. What you just won’t accept, no matter how good everything else seems.
For me, it’s honesty. If an employer is not being truthful, or not acting in good faith, there’s no chance I’d want to work there.
(Note: recruiters lying is par for the course, unfortunately. So this rule can’t be applied to them.. I’m talking about actual company employees)

story time:
I once interviewed for a small startup, who offered 25 PTO days a year – surprisingly above the normal 22 days.
When they sent over the contract for me to sign, however, I noticed that it stated 18 days off. When I questioned this, they said that the 25 days figure included the 7 national public holidays.
I’ve never heard of anyone calculating PTO that way. They were obviously trying to inflate their numbers to look more attractive. Even the recruiter was surprised.
And what do you know – I lived to regret not walking away from that job right there and then.

What are you trying to assess?

There are generally a few criteria that make for a good, or horrible, work experience.

The company culture – what do they value and reward?
How they treat employees – do they make an effort to keep employees happy?
Day-to-day work – what do you actually do 9 (or 12) hours a day? and who with?
Long-term future at the company – what can you expect after having served for a few years?

Different people will be looking for different things in each of these categories. (e.g company culture – relaxed? friendly? high-intensity?, day-to-day work – latest tech? fast paced? slow and steady?)
The first thing to do is to consider what you’re looking for in each of these areas. It would be good to also rank them, so you know which are more important.
For example, I prefer to work with statically-typed, compiled languages, especially C#. But, a friendly, supportive and relaxed culture is even more important to me than that.
That’s why I’m now with a company that is incredibly supportive and friendly, which I love, working in ruby on rails, which I really don’t.

Once you have a prioritized list, you can set about trying to assess potential employers based on it. For each category, let’s see how:

Culture

There are many different opinions of what culture is (personally, I think of it as the house rules, or ‘how things are done around here’). But there’s widespread agreement on what culture is not – pithy creeds printed on posters.
Try to disregard company “mission statements”, mottoes and the like. They’re what companies want you to think they’re all about. They don’t necessarily reflect reality.
(remember – “don’t be evil” is the motto of the world’s richest spying agency)

Questions / observations to gauge the company’s culture –

“What does it take to succeed here?”
I like this one because, usually, the answer to “what’s the company culture” is a regurgitation of company PR.
But, phrased like this, an honest answer would reveal the behaviour that the company actually rewards. It may or may not be what management says they value.
“What do you do for fun?”
The details aren’t important here. Going for drinks vs playing football in the park – that doesn’t matter.
What you’re trying to understand here is: Does the company care about employees as human beings, or as cogs in a wheel?
Do employees want to socialize with their colleagues? Meaning – do they have good relationships?
“Do you like working here, and if so – why?”
Very open-ended – it gives employees of good companies a chance to show off all of the good stuff.
In other cases, it gives you a chance to watch your interviewer squirm as they try to come up with a diplomatic answer.
“What would you improve at the company?”
A complementary question to the last one. Don’t expect to get all the dirt on the company. Instead – observe the answer. Is it honest and truthful? Or some obvious platitude?
That will tell you a lot about whether employees feel free to speak openly at this company.
“How’s the work-life balance?”
In case, like me, you care about having a life outside the office. “we work hard and we play hard” == “you work hard while we play hard”. Run away.
“How are differences of opinions settled?”
This can tell you whether the company is very hierarchical. How openly employees are able to communicate. And generally – how communication works in the organization.

How they treat employees

Look at the facilities
I read an account by a woman who, at every interview, would go to the ladies’ bathroom and look at the tampons they had. If they were the horrible cheap ones – she’d know that the company doesn’t care about employees’ welfare.
You can tell a lot just by looking around – are the office and bathroom clean and well maintained? are employees provided with an adequate working environment? are everyone sat squished together in an awful open-space? do they have access to a decent coffee machine, or are they only provided with some plastic-y coffee vending machine? All these are an indication of how much the company values its employees.
Note: Especially in tech, try to look beyond the gimmicks. A ping-pong table and a fridge full of coke may be nice, but ultimately don’t make or break your working experience. Dig deeper.
“How do leaders support employees?”
What you’re actually asking is: “Are employees considered drones, here only to serve management? or do you actually care about their needs and wants?”
“What’s the employee turnover?”
Mostly you won’t get a straight answer here, but observing the subtext of the answer can be useful.
What’s the interview / onboarding process like
Do they demonstrate a lack of respect for your time? (hours and hours of interviews, hours of unpaid work aka “take home tests”)? Are they testing for skills that aren’t relevant to the job? (remember “cracking the coding interview”?)
An interview is a two-way street, and the company knows that they have to make a good impression too. If they screw up when they’re supposed to show their best side, do you think things will be better when you’re actually employed there?

story time:
I was once interviewing with a startup that seemed very nice. I met with the engineering manager and several engineers, who all made a very good impression.
I accepted an offer from them, and was requested to start about 4 weeks later. I wasn’t working at the time, so I requested to start sooner.
The engineering manager told me to contact HR; HR told me it was up to the engineering manager. I went back and forth like this for a while. In the end, I couldn’t get an earlier start.
On my first day, they didn’t have my laptop ready – I had to spend a couple of days shadowing another engineer instead.
They also had trouble finding a place for me to sit – I ended up with a weird desk-sharing situation.
All the above are certainly annoying, but, I kept thinking “This isn’t really a deal-breaker.” Especially considering the great impression I got from the engineering manager.
I later learned the reasons for these differences: The engineering manager was desperately trying (and ultimately, failed) to maintain his department as an island of sanity. If only I had put all these signals together, I would’ve saved myself the misery of working at a certifiably pathological organization.

Day-to-day work

The culture and behaviour of the organization at-large are important. However, you’ll be spending most of your time with a small number of people, doing a certain type of work. You want to make sure that both fit you.
Remember the saying: ‘People join a company, but they quit a boss’. A company can have the most amazing culture, benefits and facilities, but they all amount to nothing if you don’t fit in with your team and the type of work that it does.

Try and actually meet with the team
see if you connect with these people. Can you imagine yourself working alongside them every single day?
For me, it’s a (slight) red flag if your interviewers aren’t actually the people you’ll be working with. this type of system means that, if you were to join the company, you’d have no say in who you work with.
On the other hand, inviting candidates to lunch / socialize with the team is a big green flag.
“What does a typical day look like?”
How many meetings / admin a day / week? are there any other duties that you’re expected to carry out?
Story time:
On my first day at this one job, my manager casually asked “By the way, do you know VB?”. Yeah, I forgot to ask about other duties, and they “forgot” to mention that legacy system that needs maintaining. Oops!
“How does a typical task go from an idea to being in front of a customer?”
Who decides what to do, who does it, how it gets approved, how it gets to the customer (CI / CD) etc.
This will give you an insight into the work process. For example – how much freedom and autonomy (and responsibility) you can expect to have. Or how up-to-date their practices are.
“What technologies do you use?”
Personally, I don’t care. I worked professionally in C#, javascript and ruby. How much I liked the work was barely influenced by the tech (except for rails, of course). But, you may feel differently.
Make sure that you’d be happy to work with the stack they have. Another very important thing is to make sure that their tech isn’t outdated.
Perhaps you’re planning a career as a specialist Fortran programmer. In that case, you probably don’t have to worry about updating your skills. Otherwise, you’d want to leave that company having acquired some marketable skills.
Look at the code
When I’m in advanced stages, I always ask to look at some production code at the company. I’d estimate that I have a better than 50% success rate.
This can give you a clue as to the standards of work at the company. (also, being willing to show the code to a candidate indicates that the people there are proud of their work! a big green flag)
“What is something you’d improve about the way you work / the work itself?”
That’s just a sneaky way to ask ‘what parts of the job suck?’

Long-term future

You may (or may not) be looking to stay at a company for a number of years. If that’s the case, you have to make sure that you have place to grow.
You want to leave a company having gained 10 years of experience, not one year of experience repeated 10 times!

“What training opportunities does the company provide?”
“How do promotions / performance reviews / pay rises work?”
For me, anything other than a clear, well-defined process means “we’ll give you a raise if and when we feel like it”.
Spoiler alert: much like your husband, they never feel like it.
“What’s the ‘career ladder’ at the company?”
For example – do you have to go into management in order to advance?
“How often does the company adopt new technologies?”
Their technology may be current today. But, unless the company makes a conscious effort to keep up, it (and you) will stay behind.

Conclusion

We all have different preferences. Different types of circumstances and environments that would make us happy.
The first step is to explicitly recognize what works for you (you will not get it right the first time – it took me quite a few different jobs to find out!).
Then, actively gather evidence about how the company ranks in these different areas. Put all of these pieces together, to reach conclusions you can be fairly confident of.
Then see how a prospective employer ranks in the metrics that matter to you.

Good luck!

Bonus question – COVID-19 response

Something I’ve seen online – a great question to ask employers in the future is around their response to the COVID-19 emergency.
Did the company take care of its employees? Was it understanding, proactive and fair to its customers? Did they take taxpayers’ money unnecessarily?
You can tell a lot by the way a person, or a company, behaves in a crisis.

Code review as a tool to improve quality. Part 3 – why should you care?

In the previous two posts we saw what feedback can be given in code reviews, and how best to provide it.
However, reviewing every (or most) proposed changes in your team is time consuming.
In this final post I will try to convince you that the effort of rigorously reviewing code is worthwhile.
I believe that code reviews are worth the effort because they are a cost-effective way to instill a culture of high quality of work in a team.

A communication method for technical leadership

An important effect of strict code reviews is: It changes the culture of the team.

Every team has a culture – the set of (usually) unwritten, unspoken rules, that the team abides by. If the reason for a team’s processes or work method is “that’s just how we do it”, then it’s part of that culture. even if that “rule” was never explicitly stated, or even discussed.

Examples of this implicit culture can be –
“only QA people do testing”, “a developer never works on more than one task at a time”, “every change must be covered by unit tests”, “we never deploy on Fridays”, “it’s OK to release code that wasn’t tested if the product manager deems it urgent enough”.

These rules evolve organically as team members observe what team management values – Which behaviour gets rewarded, and which gets punished (or is not allowed).
And these implicit rules are much stronger than anything that is said explicitly:
How many times did you hear from management that “quality is our top priority”? How many times did you then go back to your desk and push out a hacky change because it was urgent?
Actions (in this case – allowing quality to suffer due to ‘urgency’) speak louder than words.

That’s why the review process is a powerful way to set a culture of high quality work. The action of rejecting low-quality changes speaks louder than any talk about quality.
Rather than talk about wanting to improve quality, we actively reject sub-standard work.

In my experience, developers pick up on that culture very quickly.
A developer may get irked when they are asked to break up a long method, standardaize a component’s API, or justify why a certain method belongs in a certain logical layer. Especially if they’re not used to these things being an issue.
However, after a few times, they will adapt. They’ll simply save themselves the time of submitting changes that are bound to be rejected.

Developers will follow those best practices not only because that’s the only way to get their work done.
But also because they’re interested in improving and leveling-up. And this review process, done with teaching and mentoring in mind, helps them do just that.

“Academic” Training can help team members recognize what constitutes high or low quality work. But how many times have you come back from training, full of wonderful new ideas, only to return to your familiar way of working in your old project?
The review process is an effective and efficient tool for making a high-quality way of work stick. This is because, unlike training, it’s baked right into the team’s processes, and it’s provided in the context of the actual work being done.
(pair programming is even better, as it allows teaching and discussion in real-time. However, it isn’t as time-efficient as reviewing code.)

Why you don’t do it – awkwardness

I sometimes ask colleagues why they don’t employ a review process as suggested in this series. By far the #1 reason I hear is awkwardness.
It isn’t that they disagree with the importance of it, or with the different guidelines; It’s the human aspect that stops them:
Conducting such a thorough review ends up with dozens of questions or requests for change. People feel uncomfortable doing that.
They worry that the code author would be insulted or made to feel inadequate by a torrent of comments.

These concerns are justified. That’s why the first post of this series is dedicated to ways of avoiding that. It’s important to ensure that feedback is provided well, and taken in context, and not as a confrontation.

I can relate my personal experience of employing this method:
At my job, I submit by far the most comments on any given PR (about 5 comments from me to 1 comment from anyone else).
However, I try to do so with empathy and sensitivity.

The feedback I receive is overwhelmingly positive.
My colleagues (and me) have a positive learning experience as a result of my questions / suggestions. They appreciate the thoughtfulness and effort that went into reading their work.
Remember that your colleagues, like you, value opportunities to learn and improve. Don’t be afraid to help them.
(In fact, if you’re in a leadership position, that’s probably part of your job!)

The role of senior leadership

Improving the team’s quality of work is one of the technical leader’s responsibilities.
If employing rigorous review is a way to achieve that, shouldn’t it be a part of every technical leader’s job? Just like coordinating the technical work on a project, or attending pointless meetings?
So why isn’t it?

Many, senior management included, regard low-quality work as an “act of god”. Something that we are powerless against. Or, at least, a problem that has so many different causes that nobody can be personally accountable for it.
The manager didn’t actually write that bad code. There’s nothing they could have done, right?
Well, they could’ve at least read that code..

Imagine explaining to upper management why certain features are delayed / buggy / unmaintainable. Imagine management asking “who authorised this low-quality of work?”
That would make not rigorously reviewing code an even more awkward situation for the technical lead. Thus helping her solve the problem of awkwardness around applying rigorous reviews :).

Series Summary

Some of the largest hurdles that teams face in trying to improve the quality of the code they produce are:
Lack of awareness (we don’t know that there’s a better way to do this),
Lack of skill (we know there must be a better way to do this, but we don’t know what it is),
or Apathy (we know there must be a better way to do this, but it isn’t important enough).
(The last one, often expressed as “time constraints”, is something that I don’t know how to solve )

As a leader (or team member), you can use the review process as a time-efficient way to address the first two issues.
Reviews are a tool to raise awareness of problems (by commenting on them). They also to help improve skill (by prompting for, or working together on, a better solution).
Most importantly – they signal that code quality is important enough, by not allowing low-quality changes to go through.

Reviews are more specific and relevant than training, as they deal with the team’s actual code. They are also less time consuming than pairing.
This allows a single person to influence greater quantities of written code.
Done right, code reviews are a tool for teaching, learning, and setting culture and standards. Give them a go!

Code review as a tool to improve quality. Part 2 – What feedback to provide

What code quality is not (AKA you’re not an inefficient, overpriced linter)

Don’t waste time checking for things that can be automated (style and formatting violations). Instead:
1. Plug a linter into your test suite / CI pipeline
2. Have a team-wide discussion about tabs vs. spaces, curly braces on their own line or not, and all that fun stuff
3. never think of it again.

Don’t distract yourself from the purpose of the current code at hand with linting.

What I would suggest you do focus on, are the fundamentals of maintainable code and design. Those include – SOLID principles, consistent architecture and design, and code smells.
I’ll briefly discuss those in this post. I’ll provide links for further reading on each topic, as each one of them is worthy of a blog post (or book) of its own. I’ll merely provide a taster for each one.

Well-known code smells

These are relatively easy to identify, so are a great starting point.
If you take just one thing from this post, take this: Familiarize yourself with some common code smells (use the excellent refactoring guru), and look out for those.
(Note: these are highly dependent on context and situation. Some smells result from fixing other smells. You’ll have to use your judgement as to what applies to the code at hand.)

Example:

public updateUserDetails(UID userId, string newFirstName, string newLastName, string newStreetAddress, string newCity, string newState)

This method suffers from the “long parameters list” smell.
A reviewer can point out that smell, and help guide the author to its solution (possibly using a User object rather than a list of fields, or a parameters object).

Well-known code anti-patterns

These are both relatively easy to identify, and removing them provides high returns.
Familiarize yourself with some common anti-patterns, and look out for those. (Some examples – pokemon exception handling, using inheritance where composition is preferable)
The fact that they are generally agreed to be “bad” will also help you convince the code author to change them.

Example:

public isValidDate(string str) {
  try {
     var dummy = parseDate(str);
     return true;
  }
  catch (FormatException e) {
    return false;
  }
}

The above method employs the known anti-pattern of using exceptions for flow control.
A review comment here might point out the anti-pattern, and ask the author how it could be changed.
(remember from our previous discussion – prompt the author, do not spoon-feed them)
A solution to the above example could be:
Move the logic that causes a FormatException to be thrown from parseDate to isValidDate. change parseDate to call isValidDate.

Well-known architectural anti-patterns

Similar to the above, these are cases where the proposed code changes go against the system’s architecture.
For example – tightly coupling modules that should be independent of each other. breaking well-known architectural principles (such as placing UI-related code in the domain model).
Example:

private submitButtonClicked() {
  var user = UserRepository.get(usernameField.text);
  updateResult = UserController.updateUserDetails(usernameField.text, firstNameField.text, lastNameField.text);
}

The above method breaks the system’s layer model. It invokes a repository method directly from the view.
A reviewer may point that violation out.
If necessary, she can suggest moving the call to the repository to a more appropriate layer (controller layer, for example).

Loose coupling

A component has, or makes use of, little or no knowledge of the definitions of other components.
Components that do not adhere to this principle make the system harder to change. That’s because changes to component A will need to ripple out to components B and C that are coupled to it.
Example:

var streetAddress = user.Address.Lines.split('/n')[0];

The above code is tightly-coupled to the implementation of the User’s address field. Any future changes to the implementation of Address will need to take this code into account.
A reviewer may point this out (with the goal of moving this logic into the Address object itself).

Single responsibility

A component should have responsibility over a single part of functionality
(should have one, and only one, reason to change).
Useful question to ask yourself as you review a component : “What’s the role / responsibility of this?”. Look for the word “and” in the answer.
Example:

class UsersManager {
  public search(string name);
  public update (string userId, string newName, string newEmail);
  public resetPassword(string emailAddress);
}

The above class is an example of a “god-object” ; it has many reasons to change.
update will change if the fields of the User object change.
search will change as we add more searchable fields.
resetPassword will change with any changes to our authentication mechanism.
A reviewer may suggest breaking up the different responsibilities into different objects. A common pattern to do that is employing domain service objects.

Leaky abstractions

An abstraction exposes implementation details that are not relevant to its interface.
Or: a user needs to be aware of the implementation details when using the abstraction.

Useful question to ask as you read the code: “As the user of this class / method, what do I have to know?” In the answer, look for anything that is related to “how”, rather than “what”, the component does.
Remember – to use a component, you shouldn’t have to know how it does its job.
Example:

class Payment {
  public charge(double amount, string stripeMerchantID);
}

Users of the above class have to know that charging a payment is done by stripe.
A reviewer may request that references to ‘stripe’ be removed from the class’s public API. A common way to do this is to –
1. Separate the creation and usage of instances of the Payment class (for example – using inversion of control)
2. configure the class at creation time with the necessary stripe credentials.

In that way, the actual usage of this class be free of references to the underlying implementation.

Sane and consistent APIs

Good and consistent API is such that:

The inputs and outputs are consistently the same structure / types.
No unforeseen side-effects (such as modifying an input parameter)
the API client doesn’t need to do special parsing to pass values in and out of the API

Examples:

newUserID = createUser(params)[0]
the createUser method’s result requires the client to do some weird parsing.
public makeCharge(Money amount, double discountAmount)
this method represent an amount inconsistently – once as a Money object, and once a plain double.

Naming

This isn’t necessarily a category in itself.
However, paying attention to how things are named can help us spot a smell of one of the other categories.
For example –
public makeChargeAndMarkAsPurchased() indicates that the method is not singly-responsible.
The name mongoCacheProvider.store(value) leaks the fact that our cache is implemented using mongodb.
The name SendRequestToStripe(amount, merchantID) indicates how the method does its job. A better name would describe what the method achieves. For example – chargeCustomer.

Test-code smells

Test code is code in your system that needs to be maintained and evolved over time. It should not be overlooked when reviewing changes.
All the above criteria also apply to test code, especially –

Single responsibility – a single test should verify one thing.
a good metric is to make sure that each test contains exactly one assertion.
Another rule of thumb is to look at how much faking / stubbing is happening in the test. If a lot of dependencies have to be faked, this may be an indication that the subject under test is doing too much.

Leaky abstractions – the test checks against the public API of the subject under test. i.e inputs and outputs, or side-effects, such as writing to DB.
look for tests that test the SUT’s implementation (i.e the SUT calls a certain method on another object)

Naming – the test description talks about the object’s public API, rather than the implementation. i.e assertReturnsANewUserRecord rather than assertCallsUserRepository

Commit hygiene

separate commits can (and should) be used to make proposed changes clearer.

Atomicity (or single responsibility)

A commit should be a small, self-contained change. Applying that commit should achieve a single, well defined, goal.
A single “implemented the whole world” commit defeats the very purpose of having commits at all.
Ideally, a commit contains a small set of functionality, along with its verifying tests.

Commit message explains “what” and “why”, rather than “how”

A commit message should convey what functionality / business goal this commit implements. When necessary, also – why the proposed changes were made in that way.

A useful trick is to write the commit message as the completion of the sentence “When applied, this commit will..”.
For example – when applied, this commit will “add support for logging in with google”.
You may also add clarifications regarding the chosen implementation. i.e “made the login functionality async, since google authentication works asynchronously”. This will clarify why the change was made in the way that it was.

Some examples that a reviewer would want to comment on:
WIP , it works, trying something – these don’t explain anything.
Deleted unnecessary code from User class – doesn’t add any information. A reader can understand that code was deleted by just looking at the code changes.
A better commit message would explain why this code was deleted. For example: Removed support for MFA explains the business functionality that’s achieved with that change.

Some VCSs support re-writing the commit history.
you can (and should) encourage your team members to do that to create a coherent story that’s easy to follow.

Summary

These are the main things I look out for when reviewing code. It may seem like a lot, but, with time and practice, they become easier to spot.
However, a rigorous review still takes up more time than you may be used to, even for an experienced practitioner.
I’ll try and convince you that this time is well spent, in the next (and final) post of the series.

Code review as a tool to improve quality. Part 1 – how to provide feedback.

Definition

A code review (or peer review) is the process whereby proposed changes to a codebase by an author are inspected by a reviewer.
The mechanics of how this is done can vary – Github has popularized the “pull request” concept, but reviews can also occur over email, or even by just grabbing a colleague to come view your code.

Goals

To make a process effective, we should first define what it is we’re trying to accomplish. Here are the goals of a code review as I see them:

Quality control
This is about ensuring a standard of code quality – not about finding bugs or mistakes.
(Although, if we review test code as part of the proposed change, then this might serve for that purpose as well.)
The reviewer tries to ensure that the proposed changes meet certain bars of quality. If it doesn’t, they may suggest ways to improve the code’s quality.
(“Code quality” can be subjective, and I’ll try to define what (I think) it means in the next posts.)
Knowledge sharing: Reviewer –> Author
The reviewer can suggest simpler, more efficient or more maintainable options for implementing the proposed changes.
This may be because they’re familiar with a pattern or a language feature that the author is not aware of. Or they may know that someone has already implemented similar code that the author could re-use.
Knowledge sharing: Author –> Reviewer
Firstly, like the above – it’s possible that the author has used a pattern / language feature that the reviewer is not familiar with. The reviewer has now learned something new.
Secondly, this is an opportunity for the reviewer to familiarize themselves with a different area of the codebase. This means that, in the future, the reviewer will be able to more easily maintain this code.
Knowledge sharing: Documentation
In some cases, the review process is done in writing (like a pull request). In those cases, the review itself becomes an artefact that can later be referred to.
For example – when a maintainer wants to understand the purpose of a certain piece of code, or why it was written in a certain way.

Some important corollaries of the above goals are –

A review is a communication method for technical leadership. By accepting or rejecting code, they signal to the team what kind of code is and is not acceptable.
A review is a mentoring opportunity. It’s a chance for the reviewer to teach the author something that they did not already know (or learn from them).

Review as a mentoring tool

This is the first area I’d like to touch – how to make a review an effective tool for teaching and learning.

1. Be kind

Developers are only humans, and are subject to human reactions. We are much less likely to take advice or feedback on board if we feel attacked or insulted.
Phrase your feedback in an encouraging and constructive way.
For example, comments such as “I can’t merge this – this change will break the entire app!” or “This code is a bit messy – Please tidy this up before merging” are both antagonizing and unhelpful.
Even if those statements are factually correct, they don’t contribute anything to solving the issue.
Comments like “Looks like these changes break the signup functionality in X scenario; could you make sure it’s fixed before merging?” or “I find it hard to wrap my head around the implementation of the email reminder functionality; could it be simplified?” are much more helpful as a first step to resolve the issue (and actually point the author to what the issue is).

2. Teach a woman to fish

As we discussed, one of the goals of a code review is knowledge sharing. Meaning – teaching the author something they don’t yet know.
Take these opportunities to teach. Don’t focus only on “fixing” the immediate problem at hand.

Firstly, explain why you’ve made your comment, and what general principle or pattern you have in mind.
For example, rather than “This class should be smaller”, explain that “This class has too many responsibilities”.

Secondly, prefer to not prescribe a solution to an issue you’ve found.
This is because we learn better when we figure out something for ourselves, rather than being instructed.
Also, force-feeding solutions might serve to demotivate your colleagues. They may feel as though you’re treating them like code monkeys, not allowing them to think for themselves.

My preferred approach is to identify the issue clearly, and prompt the author to find a solution.
Examples include “This method seems a bit long; would it be possible to shorten it?”, or “Looks like this code is being repeated in both the DBConfiguration and XmlConfiguration classes; could we DRY it up?”

There may be cases where you do choose to suggest a solution. Make sure to explain why you’re suggesting a certain change, and what general principle or pattern you have in mind.
For example, rather than simply instructing the author to “Remove the duplication in the DBConfiguration and XmlConfiguration classes by extracting everything except the actual data fetching into a base class, which defines a virtual getConfig method that will be implemented differently by both subclasses” , explain what the “template method” design pattern is, and when it’s appropriate to apply it.

3. Seek to learn

Ask questions to make sure that you have a good understanding of the proposed changes. Ask the author why they chose a certain implementation over another. Ask about anything that seem non-standard, or that you haven’t seen before. Ask if there’s anything you don’t understand. Asking for clarifications achieves multiple goals –

It verifies your understanding of the code (meaning – you’ll be able to more easily maintain it in the future)
It challenges the author to re-think and justify their choices – They may find a better alternative in the process.
It helps you learn from the code.

Conclusion

Remember that a code review is not just about this particular change.
A review is one of the easiest, lowest-cost teaching opportunities you can get.
Approach your review as starting a conversation rather than a binary approve / reject scenario.
Many times I’ve found that I, as the reviewer, have learned something new as a result of that conversation.

Why we all choose to not pay back tech debt

“I’d love to improve this design, but I just haven’t got the time”
“I really want to tidy this up a bit more, but it’s not in the scope of this task”
“We don’t have time to re-think the architecture of this module right now”

Every developer has heard those phrases a hundred times. Hell, every developer has said those phrases a hundred times.
And, as much as we hate to admit it – many times it’s the right thing to do.

I would’ve loved to be payed to deliver elegant, beautiful code, but the reality is, my employer pays me to deliver functionality that is useful for them and for their customers; AKA Value.
Focusing on value delivered to the customer is the biggest emphasis of modern tech businesses, becoming even more prominent following the popularity of Eric Reis’s “The Lean startup”, and the whole “Tech product development” school of thought it inspired.

However, we all (including Reis & friends) also recognize that focusing entirely on customer-facing functionality is a mistake; This is where you get into the oh-so-familiar “tech debt” zone, and then “tech bankruptcy”, leading to the dreaded, and wasteful, admission of failure – the “complete re-write”.

Therefore, we can generally agree that, as software developers, Maintenance, i.e keeping the codebase in a healthy state, is an important part of our job.

But if we all recognize that keeping a healthy codebase is an important part of our job, how do we so often find ourselves with our “value” dials at 100%, but our “maintenance” dials at (nearly) 0%?
We know that maintenance is a must. Our tech lead knows it. Our CTO talks about it, sometimes even the CEO is familiar with the concept of “tech debt” and how terrified the techies are of it.
Yet, on the ground, priorities remain the same. “I don’t have time to fix it ~~right now~~ ever”.

Is there a legitimate reason?

The most popular explanation is the “Big Bad Product manager” theory: I don’t get to do technical maintenance because the evil business person keeps pushing features on me.
I find this explanation insufficient, for two reasons:

Firstly, contrary to how we like to portray them, product managers are reasonable human beings. They, too, know and fear tech debt, and they want to avoid it.
And even if they don’t, keeping the technical side of the business healthy is what your tech leadership team is there for.

Secondly, as we observed above – maintenance is part of your job.

Since when do you have to ask for permission to do your job?

A doctor won’t forgo thorough examinations and tests because “we promised the client we’d be healthy by the end of the quarter”.
A professional contractor won’t start laying down foundations before she’s verified that the ground can sustain the structure she’s building.
So why is it different in software?

A lot of the reason lies in the extreme immaturity of our industry.
We don’t have enough professional standards in place to make code health a non-negotiable part of the process, like safety checks in construction, or sanitization in health services.
(This, in turn, is because haven’t demonstrated enough professional ability to justify such standards; i.e – We took the time to ‘repay tech debt’ in the last project, but it sill turned out crap. So how can we credibly demand to do the same thing in the next project?)

(Of course, this observation will not be news to anyone who listened to what Uncle Bob Martin, and many others, have been saying for years now)

But I believe that this not the whole story – it’s not just that we’re not good (i.e professional) enough to do maintenance properly. Part of the problem is that we don’t even try.
(Remember – it’s “I didn’t do it”, it’s not “I tried, but failed to do it”)

Why don’t we even try to do maintenance?

Let’s consider your typical developer. In any given day, they have the choice to deliver either value or maintenance. We know that they (nearly) always choose the former over the latter. Even though their tech lead, CTO, colleagues, all talk about the horrors of tech debt every single day.
Why is that?

How many times have you (or a colleague) been praised for delivering a feature that was very beneficial to your clients?
Now, compare that to how many times you (or a colleague) have been praised for refactoring, maintaining, or adding technical documentation to your project?

Or, from a different angle – what do you think would be the consequences for you, personally, if you fail to deliver one, out of 100 planned features, to your client?
Compare that to what you think the consequences would be for you, personally, if you fail to improve / maintain the codebase in 100 out of 100 opportunities.

If your workplace is even remotely similar to any workplace I’ve ever seen, you know that delivering excellent maintenance might gain you the gratitude of your coworkers, but delivering features will gain you a promotion.
Which would you pick?

This conclusion is hardly surprising or novel; if there’s one thing I know about market economy (and there is only one thing I actually know), is that people will optimize their behaviour based on the incentives presented to them.
Pay developers by line of code? You get the same functionality written x10 more verbosely.
A bonus for every bug fixed? You get bugs planted in the application, so that they can be fixed later.
No tangible reward at all for maintenance? You get tech debt.

Walk the walk, don’t talk the talk

How can we move the ratio for developers away from 100% value – 0% maintenance?
Simple – managers need to put their money where their mouth is;
Stop talking about wanting to tackle tech debt. Instead, show that you’re willing to pay people to tackle it.
(Balance is important here; while we don’t want the split between value and maintenance to be 100/0, a 50/50 split is also unreasonable)

How about having maintenance-related line items as part of your yearly review process? As part of the developer role specs?
Why not, for every 10 times you ask “how’s such-and-such feature coming along?”, ask “how have you improved the code lately?” once?

Once people understand that there’s an incentive for them to keep the codebase healthy, that their promotion, raise, status within the organization, is affected by it, they will find the time, scope, and energy, to get it done.

(*Huge caveat: To effectively judge whether someone is achieving a certain goal, you need to be able to measure it. Measuring code quality, or the amount of improvement of code quality, is not a solved problem by any means. It’s a subject worthy of a blog post (or book, or academic research) of its own.)

What do you think? Does this description map to your own experiences? Is there a legitimate reason why we’re prevented from paying back tech debt that I’ve missed? Or maybe this isn’t actually a problem at all? Let me know below.

Inheritance is a hammer. Eliminating code duplication is not a nail.

It’s a daily occurrence when adding features to a system –
we currently have functionality X implemented, but now we also need functionality X’, which is just slightly different.
Re-implementing most of X with some modifications is code duplication, and this is bad^TM.
We need to keep our code DRY, which means that both X and X’ need to share some code.

For many, especially in certain communities (ruby), the ~~first~~ only solution they’d consider is:
“Let’s make X’ inherit from X”
“That way, we can change some of the functionality in X’ by overriding some methods / properties”.

This isn’t always the best solution.
Let’s look at the (over-simplified) example of this service, which handles the process of creating a new user record:

class UserCreator

  def create_new_user(username, password)
    existing_user = User.find_by(username: username)
    return 'user already exists' if existing_user

    return 'password not complex enough' unless password =~ SOME_UNGODLY_REGEX

    user = User.new(username, password)
    user.save
    send_welcome_email(user)
  end
end

And now we have a new requirement: we want to be able to create a user of type AdminUser as well. Also, the welcome email for an admin user is different.

Well, we can’t be repeating all of this code again, with the only difference being AdminUser instead of User, and some email wording, right?
Many times we’d find ourselves with this solution:

class UserCreator

  def create_new_user(username, password)
    existing_user = user_class.find_by(username: username)
    return 'user already exists' if existing_user

    return 'password not complex enough' unless password =~ SOME_UNGODLY_REGEX

    user = user_class.new(username, password)
    user.save
    send_welcome_email(user)
  end

  private

  def user_class
    User
  end
end

and then:

class AdminUserCreator < UserCreator

  private

  def user_class
    AdminUser
  end

  def send_welcome_email(admin_user)
    # some fancier email here
  end
end

Simple enough, right?

This is where sirens should start flashing: AdminUserCreator has only private methods and no public interface?? What’s the justification for that?
Seeing this unusual situation alerts us that our design may be lacking.

Looking more critically at our solution, we can spot a few issues:

Single responsibility is broken

As we all remember, single responsibility means that an object has only one reason to change.
In our case, UserCreator should only change if we change the way we create users.
But, we just made quite a lot of changes to UserCreator to accommodate the way we create admin users.
Meaning: UserCreator has two reasons to change.

Encapsulation is broken

As we can clearly see above – UserCreator is accessing private members of AdminUserCreator . One class interacting with another class’s private methods isn’t an ideal situation.
Worse – the other way around is also true – AdminUserCreator can also access the private parts of UserCreator!

Which leads us to the next point:

Interfaces are broken

Take this exaggerated example: the welcome emails for the two types of users are completely different, but the email title is similar. What would that look like?

class UserCreator
  # our public methods

  private

  def send_welcome_email(user)
    title = get_title(user)
    # more email generation...
  end

  def get_title(user)
    friends_stats = get_friends_stats(user)
    return "Welcome #{user.name}! you have joined   #{friends_stats.number_of_friends} of your friends who were active #{friends_stats.activity_last_day} times today"
  end
end

class AdminUserCreator < UserCreator

  private

  def send_welcome_email(user)
    title = get_title(user)
    # admin email generation - something completely different...
  end

  def get_friends_stats(user)
    # stats for admin users are calculated differently
  end
end

Our code is DRY, and that's what's important, right?

Let's try, together, to follow the control flow of sending an email to an admin user:

1. Parent's create_new_user is called
2. Parent’s create_new_user calls Child’s send_welcome_email.
3. Child’s send_welcome_email calls Parent’s get_title.
4. Parent’s get_title calls Child’s get_friends_stats.

Confused? So are we.
It’s impossible to understand what a single method does without looking at both classes, and having to guess, at each method call, what actually is being executed.

Look at AdminUserCreator again – it only has some floating methods without any context that are impossible to understand (you can’t tell when or how get_friends_stats is called, for example, because it isn’t called in the class you’re looking at!)

(And just imagine what would happen if we added SuperUserCreator to the mix!)

The issue here stems from the fact that we have two classes that interact with each other, but it’s impossible to know when these interactions take place:

The interfaces between these two classes are inferred (based on whether a certain method has been overriden or not) rather than explicit (based on defining, at the call site, the object on which the method is executed).

Single responsibility is broken

Wait, didn’t we just talk about this..?
Well, it turns out, we can break this principle twice:

Since the child class has no choice but to inherit all the implementation (and therefore – responsibilities) of the parent class, if we add anything to it, it now has multiple responsibilities – its parent’s, and its own.

Imagine that, when an admin user is created, we want to take some more actions – send an email notification to other admin users, create a special log entry to document it, etc.

Because of the way the parent and child classes are all jumbled together, the child class now has more responsibilities than we would want, and we can’t easily extract them, because another class, and a completely different functionality, is involved.

Duplicating responsibilities also leads to –

Test code is broken

Two classes having the same functionality means that we have to have the same set of tests for both of them, to verify this functionality.
This causes either code duplication, or having to share test code (sharing test code in an easy to understand way is something that I find very difficult to do).
Additionally, the differences between the parent and child classes are in their private methods. That can be sometimes awkward to test.

What’s the alternative?

Prefer composition over inheritance. Meaning – compose the different responsibilities using a third, collaborating object that’ll take on the shared responsibilities, make it configurable as needed, and have both implementations use it.

In our example we can consider two such collaborators:

class UserEmailTitleGenerator

  def get_title(user, friends_stats)
    "Welcome #{user.name}! you have joined   #{friends_stats.number_of_friends} of your friends who were active #{friends_stats.activity_last_day} times today"
  end
end

class UserRecordCreator

  def create_user_record(username, password, user_class)
    existing_user = user_class.find_by(username: username)
    return 'user already exists' if existing_user

    return 'password not complex enough' unless password =~ SOME_UNGODLY_REGEX

    user = user_class.new(username, password)
    user.save
    return user
  end
end

and now our original classes become trivial:

class UserCreator

  def create_user(username, password)
    user = UserRecordCreator.create_user_record(username, password, User)
    send_welcome_email(user)
  end

  private

  def send_welcome_email(user)
    user_stats = get_regular_user_stats(user)
    title = UserEmailTitleGenerator.get_title(user, user_stats)
    # ...
  end
end

class AdminUserCreator # look mom - no relation to the other class!

  def create_user(username, password)
    user = UserRecordCreator.create_user_record(username, password, AdminUser)
    send_welcome_email(user)
  end

  private

  def send_welcome_email(user)
    user_stats = get_admin_stats(user)
    title = UserEmailTitleGenerator.get_title(user, user_stats)
    # ...
  end
end

..And it’s that stupidly simple. Instead of configuring the two classes differently by overriding methods and attributes, we configure some lower-level services by providing different arguments.
(*in fact, the above example is so simplistic that it can easily be solved by a single class using inversion of control; I’ll leave that as an exercise to the reader)

This approach avoids the aforementioned disadvantages –

It keeps true to SRP – since the two Creator classes are completely unrelated, and can only change independently.
It maintains very clear interfaces – for each method call, we know exactly what method is executed, and against which object, and with which parameters.
Single responsibility is, again, maintained – because no class needs to assume responsibilities that are not strictly its own.
In fact, this approach helps us spread the responsibilities in even finer-grained detail; we can have a class for just creating the user record, a class just for the email generation, etc..
Since common responsibilities are now extracted to a completely separate class, they can be tested only on that class (i.e creation of the user record can be tested only for the UserRecordCreator class, and not for UserCreator or AdminUserCreator.)

There are no free lunches, of course, and these advantages carry with them a new disadvantage –
we’ve now broken up our functionality into many small pieces. These pieces may be too small, and having many small pieces can be very very confusing.

Summary (or: are you telling me that I should never use inheritance?)

No.

I’m telling you that inheritance is one tool in your toolbelt, and it has its uses. But it isn’t the right tool for every job.

Like almost any design decision we make, it all comes down to tradeoffs. Each approach has its own pros and cons.
My thinking is that, even if inheritance doesn’t immediately show all of the cons listed above, starting down that path makes it hard for me to change course and start breaking things up, because by nature inheritance couples my classes quite tightly together.

That’s why I’d usually use inheritance only in these very specific cases:

Is-a relationship: Like in the classic examples we’ve all seen in school – a car is a vehicle, because it has the same properties (move, make_noise), but some different implementation, or maybe some additional properties / methods.
(meaning – it needs to meet Liskov’s substitution principle – any code that expects an instance of parent can equally use an instance of child).
Note that this narrow definition is almost always restricted to domain classes only;
While it’s easy to imagine that a triangle is a shape, it’s hard to say that “monthly report controller” is a “report controller”.
Well-known design patterns: Several design patterns utilize inheritance. I’m very comfortable using inheritance in these cases because it’s very clear what’s going on and how the different classes interact (because the patterns are well defined)

But, in most other cases – I’d look elsewhere than inheritance.

Rails lessons learned: Fat Controllers and Obese Models

So it’s been nearly a year since my rant on rails. In this year I’ve been working on a rather large rails application, trying to help the team tame this 100Ks LOC monolith.

We’ve learned a lot about what wasn’t working for us with the out-of-the-box rails architecture, and how to try to work around that.

In this (and subsequent) posts, I’ll try to describe the issues we’ve faced, and our ways of trying to improve things.
Let’s start with one of the most common issues rails applications face:

Fat Controllers

Too much responsibility in controllers is a very easy pit to fall into in rails application.
I believe that this is because controllers come pre-loaded with so much rails magic and functionality, that it takes very little to get them to be doing ‘too much’.

We all know that controllers should only concern themselves with receiving data from the client and passing data to the views. Like so:

class SkinnyController < ApplicationController

  def create
    result = ...# do something with params[:some_model]
    if result
      @entity_to_show = # get whatever it is we want to show
      @another_param_for_the_view = ...
      render 'some/partial'
    else
      render 'something/else'
    end
  end

end

Things in real life, however, don’t always look like the ‘hello world’ example from RailsGuides.
In real life, we have requirements such as:
1. when an Order is purchased, a payment is taken, the nearest warehouse that has all of the OrderLine items is notified, and an email is sent to the client.

2. the user shouldn’t have the option to pay for the Order if any of the OrderLine items is not currently in stock.

Where do we put all of this logic?

Traditionally, developers would recognize that taking payment and sending emails aren’t domain model responsibilities.
Additionally, signalling to the view which options should be shown, seems to be appropriate for the controller.
So, the first solution most of us would come up with is sticking all of the above logic in the controller.

This is of course a Bad^TM thing.
Firstly, because It puts business logic into the controller, which couples business rules and presentation (Bad!).

Additionally, a point that isn’t immediately evident regarding controllers, is that they’re harder to test than other types of classes.
Therefore, we would like them to have as little logic as possible, because any logic that they contain would be difficult to validate.

The reason why controllers are hard to test is that they don’t have a very well defined API – they just send some messages to a bunch of models, and then render something.

Testing interaction with model classes

Requires us to either:
1. stub them (discouraged for non-statically typed languages, since it’s much harder to make sure the stub’s interface matches the actual class’s interface:
expect(card).to receive(:pay).with(amount, cvc) might pass, even if the pay method actually takes the cvc as a first argument, and amount as the second)
Or
2. read objects from the database and verify them (which poses 2 problems – 1. performance, 2. We’re now testing the model classes, which is not the point of those tests),

This quickly gets very messy the more logic you add on.

Testing that the correct data is rendered

Requires us to either:
1. test the value of the @show_payment_option? instance variable (Problematic – we don’t want to test private attributes)
Or,
2. test the rendered view (Problematic – coupling our controller test with the partial implementation, and introducing more complexity by having to test generated HTML)

For the above reasons – the controllers’ high coupling with the view, and their reduced testability, rails devs are commonly told to aspire to have
“Skinny controllers, fat models”
which is to say –
take as much responsibility away from the controllers, and put it into the models.

Obese models

take as much responsibility away from the controllers, and put it into the models.

As we saw in the previous section, the first part of this sentence is absolutely true; we don’t want a lot of responsibility in our controllers.

However, just blindly sticking all of it into the model classes isn’t great either.

Let’s go back to the 2 examples from before:
1. the system needs to take payment, notify a warehouse and send an email when an Order is purchased,

2. the user shouldn’t have the option to pay for the Order if any of the OrderLine s is not currently in stock.

The first requirement obviously does not belong in a single model class; it concerns many model classes, and possibly some other services.

However, even the second, simpler requirement, is tricky.
At first glance, it makes sense to have a can_pay? method in the Order class.

But, how can the Order know whether a specific item is in stock? Does it need to fetch it from the database? query the Warehouse class? call the external StockManagement service?
Even the simpler case of simply fetching the Item from the database and querying it breaks the isolation of these two classes from each other.
The other described cases are even worse, of course.

Moreover, it’s expected that we would have many more requirements to take some action based on the state of the Order object:
Show the order number in red if there’s an unhandled Complaint attached to it and the order total is more than $100,
stop the order from being shipped if a previous order to the same address was not picked up,
and whatever else your friendly product manager can dream of.

Putting the logic for all of those different things into the model class would create an obese class, littered with has_pending_complaint? , has_previous_denied_shipment? etc., each of them, potentially, concerned with more than just the Order object itself.

So where does business logic, which doesn’t belong in a single Model class, go?

Domain services

This is a well-known concept in other technologies, however, it doesn’t immediately spring to mind when using rails, simply because it’s not a “thing” in rails.

The point of a domain service is to perform an operation which requires interaction with more than one domain object.
The domain service is still in the domain layer, like model classes.
However, it doesn’t hold any data of its own (and it’s not persisted), but rather it is concerned with coordinating several domain classes to represent a process, or a workflow. For example:

class OrderPurchaseService

 def pay

  payment_result = @payment_gateway.pay(@credit_card, @order.total)
  return false unless payment_result

  nearest_warehouse = Warehouse.has_items(@order.order_lines).nearest
  nearest_warehouse.prepare(@order)

  ConfirmationMailer.new(@order).send
  # Whatever else needs to happen here
  return true

 end

end

(For a more detailed description of domain services and their relation to DDD, see here)

Abstracting a business process into its own class / method helps us solve the issues we discussed previously:

It’s better than having the code in the controller, since now these business rules are removed from the presentation concerns, and have a well-defined output which is more easily testable than a view output.

It’s better than having the code in the model class, since it prevents one model class from being too familiar with other model classes, and stops model classes from implementing methods which aren’t directly related to the model realm (such as has_pending_complaint?)

The controller’s job can now be described as gathering the objects to pass to a domain service (either from the received params, or by reading them from the database), invoking the appropriate domain service, and acting on the result.

The model’s job can now be described as defining the structure and operations over a single domain object.

The Domain Service’s job is to achieve a business goal by using several domain objects / services.

Bonus: Query classes

Query classes are a subset of domain services; these are domain services that don’t cause any changes, and are only used to calculate a result.
Sort of a read-only domain service. (The name is taken, of course, from the Command-Query pattern).
The above-described use cases, such as has_previous_denied_shipment? and has_pending_complaint? can be abstracted into query classes in order to help keep the models from becoming obese.

Conclusion

Rails does not presume to offer all the tools and paradigms required for any application.
It gives a very good starting point for quick and simple applications.

As your application grows, however, the simple 3 concepts of an HTML-view, a controller and a model will struggle to contain all of the more complex logic you’ll need.

In the case of a processes, which require several model classes to cooperate, abstracting it into its own class will help keep your controller and model code singly-responsible and well-isolated.

Angular 1.5 starter kit

This is an introduction to my angular starter project, explaining what it is, what it contains, and why it was built.

This project is a fork of angular-starter, changing it a little bit, and adding some more features and samples to it.

Why Angular 1.x?

It’s true that Angular 2 is now all the rage. However, I think that there are still valid reasons to be starting a new project with Angular 1.5:

Even though it seems like it’s been around forever, Angular 2 is still, currently, not released yet.
I’ve heard the angular team say that there are plenty of applications built on Angular 2 beta running in production, but there are many (including me) who wouldn’t use anything pre-release in production.
Even after it’s released, the resources and help you can get for Angular 1.x are much much better than for 2.0.
Even though the angular team has done a great job in churning out documentation for version 2, you’re still likely to struggle to find answers and non-trivial examples for angular 2.
This is especially relevant for those who want to create a non trivial angular app that’s more than just forms and CRUD; they will struggle to find examples and solutions for complex problems in angular 2.
There are many teams with existing skills in angular 1. For them, the learning curve of angular 2 might not be worth the switch (obviously, depending on the expected scale and lifetime of the project).

Why another startup project?

True, there are already many. However, I wasn’t able to find one that answers all of my requirements:

Modern (i.e using ES2015 and angular 1.5 with components)
Production-ready (webpack configuration for release)
Follows best practices
Non-trivial (provides working examples of more advanced use cases)

The original angular-starter project covers the first 2 points very well, however – I felt it could be improved in the latter 2 categories.

What it is

Modern

Obviously, this project uses the babel transpiler and webpack modules, which allows you to use ES2015 features , as well as angular 1.5, and SASS compilation.
It also contains example code for using angular’s new component .

Furthermore, it uses ui-router for routing, and ng-annotate for automatically injecting dependencies in a minification-safe manner.

this is currently the recommended way to write angular 1.5 apps (at least until next week, when something better comes along :p ), and I’ve yet to see a starter project that features examples for all of these.

Twitter bootstrap and FontAwesome have already been included, since they’re so widely used.
(If you’re not interested in using them, it’s as easy as simply removing them from the package.json and from your .scss files).

Production-ready

The project’s build script is already configured to concatenate, uglify, and minify your code, with cache busting, ready to be served in production.
The resources directory is copied as-is to the production build, so that’s the place to keep your resources (i.e images).

Follows best practices

Sadly, John Papa hasn’t updated his famous angular 1 style guide to include ES2015 or components.
Thankfully, Todd Motto has stepped up with his own style guide covering modern angular 1 apps. My project tries to follow that guide, especially module structure.

Linting: The project comes with linting using ESLint, and is pre-configured to use the recommended rules.
You’re encouraged to add or modify to them (in the .eslintrc file) as appropriate for your team.

Non-trivial

I found that many samples and starter kits don’t contain any examples of using dependencies, or testing any type of realistic scenario.

This is why I’ve added the home component, which uses a service as a dependency, and has a route that leads to it.
This allows you to see an example of routing to a component, and of using dependencies and the 'ngInject' directive.

In the tests for this component, you can see how to mock its dependencies using the $provide provider, and how to test using a mock that returns a promise.
I’m also using the inject function to obtain a reference to the $componentController, which is used to create instances of components that you can run tests against.

To understand more about mocking and testing, I recommend reading the jasmine docs.

Summary

I’ve tried to create a starting point that is ready to go.
The idea is to allow developers to clone this starter project, and immediately start writing their application code, without having to spend any time configuring frameworks, tools or dependencies.
I’m completely comfortable with taking this project and starting to build a production application with it (in fact, I have done this), and I hope that others will find it useful as well.

If you have any comments or ideas for improvements (hey, I’m just learning as I go along..), feel free to open an issue (or even better – a PR) on the repository.

The 5 stages of developer grief

I got to thinking recently about the relationship between a developer and his / her code.

I’ve come to the conclusion that this relationship is parental:
Your code is, figuratively, your ‘baby’.

It’s something that you’ve created, and that you’re proud of. You’d do anything to protect it.

Before you dismiss this as crazy, new-age hippy talk, hear me out; I believe I’ve got a point here.

Like any scientific hypothesis, we don’t only expect it to explain some known phenomena (more on that later on), we also expect it to be able to illuminate and explain other, more subtle, behaviour.
Let’s try that.

So if we accept, as a working thesis, that we treat our code like our baby, it means that a bug found in our code is like being told that our baby has some sort of a horrible disease.
It’s shocking and heartbreaking news.

150217_dx_vaccineresist-crop-promo-mediumlarge — “Your little girl has a bug in her command-line interface. I’m afraid it’s terminal.”

And, we would expect, that we respond to this kind of news with the appropriate feelings of grief and mourning. Let’s see.

Stages of grief

True story:
Earlier in my career, I was a team lead for a fairly large team (around 7 devs).
Me, and the developers who were more junior / new to the team in one office (so that they can get assistance from me easily), and a few more senior team members in the other room (free from the ‘pestering’ of the juniors).

One of our senior team members in the other room, let’s call him ‘Jerry’, was a super talented developer, although a bit young and rash at the time.
He’s a really stand-up guy, also with a great sense of humor, and not afraid to show his emotions.

I’ll try and transcribe here a typical chat log of banter between Jerry and me (yes, we used chat to communicate from the other room. After all, laziness is a virtue :)), that demonstrates quite nicely the different stages of grief Jerry went through whenever a bug was found:

Jerry> Finally! just finished and pushed the changes to the reports. Yay!
Me> Cool man! well done. I’ll take a look in a second.

<5 minute pause while I check-out the code, fire up the system and play around with it>

Me> It doesn’t work.

Stage 1: Denial

In this stage individuals believe the diagnosis is somehow mistaken, and cling to a false, preferable reality.

Jerry> What do you mean? of course it works, I tested it.
Me> Nope, I’m getting an exception here.
Jerry> Are you sure you have the correct branch checked out?
Me> Yes, I’m sure; I can see the new button you’ve added here. And I’m getting an exception.
Jerry> That’s impossible! Show me!

Stage 2: Anger

When the individual recognizes that denial cannot continue, they become frustrated.
Certain responses in this phase would be: “How can this happen to me?”; ‘”Who is to blame?”

Jerry (storming into my office, destroying everything in his wake, throwing terror into the hearts of the more junior developers): How can you be getting an exception??!? I’m telling you I thoroughly tested this code!!
Me (showing on screen): See, if you select a filter before clicking ‘generate report’, the whole thing crashes.
Jerry: Oh, filtering! I forgot about filtering!! Why do I always get these stupid edge cases??!!

<1 minute pause as Jerry contemplates the exception on the screen>

Jerry: A-ha!! I’ve GOT it!
I’ll tell you what – It’s that idiot <insert name of a developer who left the company>’s fault! He did such a lousy job with the filtering, no wonder it crashes!

Stage 3: Bargaining

The third stage involves the hope that the individual can avoid a cause of grief.
People can bargain or seek compromise.

Jerry: Wait a minute, hang on! But why would anyone use a filter before generating a report?? That doesn’t make any sense! They would never do that! Nobody does that. Why would they do that???
Me: Well, it looks like a pretty legitimate workflow to me…
Jerry: Look, I know Morty and Helen (our main customers and power users). They never filter a report before generating it.
In fact, they never even filter a report at all! I bet you that no-one even uses this stupid feature anyway!
You know how this filtering thing came about? It’s because Helen was able to do filtering in Excel, so she thought that our system must be able to do it too.
But they’ve never used it since we developed it!
Why don’t we just remove the filtering option here, and everything will be ok?
Me: Jerry, we’re not going to remove this feature.

Stage 4: Depression

“I’m so sad, why bother with anything?”. During the fourth stage, the individual despairs.
In this state, they may become silent, mournful and sullen.

Jerry ( miserably retreating back to his office): Fine, I’ll fix it…

Jerry (indistinctly, from the other room, talking to a colleague): You know what the problem was? It was <developer who left>’s stupid filtering! And now I have to fix it! What’s the point of developing new features if I have to keep going back to fix someone else’s mess??

Stage 5: Acceptance

<30 minutes of debugging>

Jerry (Clearly heard screaming from the other room): OOOH!! NOW I’ve GOT it! Of course!!! What an idiot I’ve been!

<1 minute pause>

Jerry> Yeah, you were right, I forgot to check the filter values before generating a report.
Anyway, I fixed it now, it’s all good.
Me> Cool man! well done. I’ll take a look in a second.

<5 minute pause while I check-out the code, fire up the system and play around with it>

Me> It doesn’t work.

etc.. etc..

So we can see clearly here that Jerry, indeed, treats his code in a very personal, emotional way.
And, although you yourselves don’t usually get genuinely angry when your code doesn’t work, or desperately try to redefine a bug as a feature, can you not see a little of yourselves, or your colleagues, in Jerry?

What else?

As I mentioned earlier, this theory can explain a few other strange ways in which developers relate to their code:

Response to criticism –
As developers, we pride ourselves on being rational creatures;
If there’s a better, more efficient way of doing something than what we’re currently doing, we would be happy to learn and improve. Right?This is where any of you who’s ever worked on a development team goes “Yeah, right…”. It’s never like that.You always have to fight tooth and nail with someone to get them to change their code, long after you’ve proven to them that your suggestion is better.
And of course, a day later, when they make a suggestion to you, they have to do the same thing.

This is because that, even though you know that your child isn’t perfect, you’d be damned if you’re going to let some stranger tell you that!

That baby is your pride and joy, and it doesn’t need any fixing.
It’s beautiful just the way ~~god~~ you created it.
Developers can’t find bugs in their own code –
How many times have you grappled with some code that mysteriously just won’t work, unable to make any progress, until a colleague passes by your desk and says “oh, you know you forgot to insert a trailing ‘;’ there, right?”. You were looking at it all that time, yet you just couldn’t see it.Researches have found that code review is an effective way to find bugs.
Even those who disagree note that 1 out of 6 review comments indicates a possible bug. That’s a lot.

So how come someone, who has less intimate knowledge of the code than I do, can find a bug by looking at it for 5 minutes, which I haven’t found looking at my own code for 5 hours?

This is because, to me, my baby is perfect. I’m so in love with it, I’m just not able to see any of its flaws.

“Yes, it’s quite breathtaking”

As a side note, this is one of the justified reasons that DHH (creator of the Rails framework) has his knickers in a bunch over TDD; You can’t expect a programmer to test their own code effectively.

Afterword (or, “so what?”)

So was Jerry a bad developer who kept pushing bugs into the codebase?
Not in the slightest. He was one of the best I’ve worked with, not the least because he had the great showmanship and sense of humor to act out this wonderful show almost on a daily basis.

Since then, I haven’t seen any developer act out their grief in such an obvious way. But if you know what to watch for – I bet you can see it with every developer, not the least in yourselves.

And this is, I guess, the moral of this whole story – if you can recognize this behaviour, you can take steps to avoid it, or counter it.

Whenever I get a comment or a suggestion regarding my code, I’m mindful that I’m inherently biased, and try to balance accordingly.
I found this realization has helped me considerably in accepting suggestions and improving my skills.

Like another great developer friend of mine once told me:

“If you don’t look at code that you’ve written six months ago and think ‘What is this crap? How could I have been so stupid?’, it means you haven’t improved.”

A useless blog from, and for, useless developers

How can you assess if a company is a right fit for you?

Evidence-gathering methods

Deal breakers

What are you trying to assess?

Culture

How they treat employees

Day-to-day work

Long-term future

Conclusion

Bonus question – COVID-19 response

A communication method for technical leadership

Why you don’t do it – awkwardness

The role of senior leadership

Series Summary

Other posts in this series

What code quality is not (AKA you’re not an inefficient, overpriced linter)

Well-known code smells

Well-known code anti-patterns

Well-known architectural anti-patterns

Sane and consistent APIs

Naming

Test-code smells

Atomicity (or single responsibility)

Commit message explains “what” and “why”, rather than “how”

Summary

Read next

Other posts in this series

Definition

Goals

Review as a mentoring tool

1. Be kind

2. Teach a woman to fish

3. Seek to learn

Conclusion

Read next

Is there a legitimate reason?

Since when do you have to ask for permission to do your job?

Why don’t we even try to do maintenance?

Walk the walk, don’t talk the talk

Single responsibility is broken

Encapsulation is broken

Interfaces are broken

Single responsibility is broken

Test code is broken

What’s the alternative?

Summary (or: are you telling me that I should never use inheritance?)

Fat Controllers

Where do we put all of this logic?

Testing interaction with model classes

Testing that the correct data is rendered

Obese models

So where does business logic, which doesn’t belong in a single Model class, go?

Domain services

Bonus: Query classes

Conclusion

Why Angular 1.x?

Why another startup project?

What it is

Modern

Production-ready

Follows best practices

Non-trivial

Summary

Stages of grief

Stage 1: Denial

Stage 2: Anger

Stage 3: Bargaining

Stage 4: Depression

Stage 5: Acceptance

What else?

Afterword (or, “so what?”)

Menu