programming-basics-materials/code-review-and-feedback.md

marp	paginate	math	theme	title
true	true	mathjax	buutti	Code review & feedback

Code review & feedback

Contents

1. Code review basics
   • What is a Code Review?
   • What is assessed in a Code Review?
2. Code review criteria
   • Functionality
   • Style and readability
   • Structure and maintainability
3. Giving feedback
4. Group exercises

Code review basics

What is a code review?

• Code Review is when other people review the code you made
  • Read the code, after or during implementation
• Designed to
  • identify bugs
  • increase code quality
  • transfer knowledge
  • increase a sense of mutual responsibility
• Common practice in software development
• Extra: It is sometimes also used to assess the programming skills of a recruitee!

What's it like

• Can be formal or informal
• Can be regular or irregular
  • E.g., can occur for every pull/merge request
• Some variations include:
  • Over-the-shoulder reviews between two developers
  • Email passaround
• There's also:
  • Tool-assisted reviews
  • Pair programming kind of applies I guess

Code review criteria

Three main criteria

• In code reviews, many criteria can be assessed.
• Today, we'll be focusing on these three:
  • Functionality
  • Style and readability
  • Structure and maintainability

Other criteria

• Other criteria include:
  • Performance and efficiency
  • Error handling
  • Security
  • Test coverage
  • Code reuse and dependencies
  • Compliance with coding standards
  • Documentation

Functionality

Why assess this?

• It's important that the code not only works in intended way…
  • The code needs to
    • meet the given requirements
    • adhere to specifications
    • pass the tests
  • To ensure this, go through edge cases and possible error scenarios
• Also, unintended functionality should not be present after delivery

Questions to use in assessment

• Does it work as intended?
• Does it take into consideration expected user interactions?
• Does it take into consideration unexpected user interactions?
• Is the expected user interaction made clear?
• Are there clear instructions, comments and documentation?

Style and readability

Why assess this?

• When you're working in a project alone, you might not usually pay attention to the style and readability of your code
  • if you do, then you also understand how difficult this can be (more on this on the next slide)
• In a professional working environment, readability is really important, because you're usually working with code that is maintained by other people as well
  • Having readable code is key, in making sure that team can function now and in the future
• Style refers to utilizing the latest features and standards of the language

Difficulties of writing readable code

• It would be easy to say that by simply adding comments, more line-breaks and more descriptive variable names to our code, readability will improve
  • But! Readable code involves much more...
  • Adding relevant comments in our code may turn the code overly commented
    • This has the opposite effect: making the code slower to read and harder to understand
  • Too long variable names can have a similar effect
  • Writing readable code is always a balancing act
    • It is often difficult to balance between readability and efficiency
• Note: Naming conventions should be followed when writing new code. Mixing these will make code less readable.

Example 1

• Bad code:

  var identity = false;
  var text = "Hello";
  var userName = "Rene";
  
  console.log(text + ((identity) ? " " + userName + "!" : "!"));
  

• Worse code disguised as better code:

  const greetUserText = (greeting, userIdentity = "", suffix = "!") => {
    const greetingSuffix = (userIdentity) ? `${userIdentity}${suffix}` : suffix;
    return greeting + greetingSuffix;
  };
  
  console.log(greetUserText("Hello", "Rene"));
  

Example 2

• Worse code disguised as better code (same as earlier):

  const greetUserText = (greeting, userIdentity = "", suffix = "!") => {
    const greetingSuffix = (userIdentity) ? `${userIdentity}${suffix}` : suffix;
    return greeting + greetingSuffix;
  };
  console.log(greetUserText ("Hello", "Rene"));
  

• Better code:

  const getGreeting = (greetWord, userName = "", punctuation = "!")  => {
    if (userName)
      return `${greetWord}, ${userName}${punctuation}`;
    else
      return `${greetWord}${punctuation}`;
  };
  console.log(getGreeting("Hello", "Rene"));
  

Example 3

Over-commented code:

/**
* Constructs a string that contains the given \`greeting\` and optional \`userIdentity\` and returns it.
*
* Eg. "Hello Rene!"
*
* @param {[string]} greeting Used in the greeting as a prefix, eg. "Hello" or "Konnichiwa"
* @param {[string]} userIdentity Optional user identity, included in the greeting. Default is empty string.
* @param {[string]} suffix Optional suffix, that is added in the end of the greeting. Default is "!".
*/

const greetUserText = (greeting, userIdentity = "", suffix = "!") => {
  // if userIdentity is true, return greeting with user identity
  if (userIdentity) return \`${greeting} ${userIdentity}${suffix}\`;
  // if it was not true, return plain greeting
  return \`${greeting}${suffix}\`;
};

/* log greeting out here*/
console.log(greetUserText ("Hello", "Rene"));

Practices for readability

• Common naming conventions
• Clear variable names
• Use of linters
• Asking someone else if they can understand your code
• Self-commenting code
• Consistency

Questions to use in assessment

• Can you understand the code at a glance?
• Is the code structured well?
  • (Do you have to jump between multiple lines to understand the flow?)
• Are the variables named clearly?
• …Is it possible to understand the code without asking the writer at all?
• Does the code have…
  • enough comments?
  • …too many comments?
• Docstrings exist/required?
• Does the code use the best practices of the language?

Structure and maintainability

Why assess this?

• Structure and maintainability are two separate concepts, but in programming, they go together
  • Well structured code makes it easier to add new features.
• Maintainability is a result of a good structure, but it takes a lot of different aspects and compromises into consideration
  • E.g.: Sometimes, making code more efficient makes it less readable, and thus, less maintainable!

What is this in practice?

• In smaller applications: Encapsulating code into clear and compact functions & classes
  • Makes the code easy to reuse and makes creating unit tests easier
• In larger applications: Application is divided into clear segments
  • E.g., file structure is easy to understand and allows for scalability
    • Easier to add new features
    • A possibility of a larger user base or larger dataset in the database is taken into account
• Part of maintainability is utilizing best practices of the programming language
  • E.g., if the language by nature enables good encapsulation, it should be utilized to the fullest (in most cases)
  • Learn the SOLID principle

Questions to use in assessment

• Is the file structure easy to comprehend?
• Is the code encapsulated in clear and concise functions?
• Is the single-responsibility principle applied?
• Does the code utilize best practices of the language?
• Can you easily add new features to the code?
• Is the code scalable?
  • What if some application state variable has a new value?
  • What if instead of one, you have many?
    • Have you considered translating the app to another language?
    • What if the login system has to be swapped to another?
  • What if, instead of 10 values in an array, you have 140?

Conclusion

• We focused on three criteria of the Code Review process:
  • Functionality
    • is the code working, is it easy to use?
  • Style and readability
    • does it use latest features, is it easy to understand?
  • Structure and maintainability
    • Can it be scaled, is it encapsulated sufficiently?
• Code Review is a common process in software development and recruitment
• Aim to always produce the best code you can in your work and free-time
  • Especially if you use said code as a reference!

Giving & taking feedback

People make code

• Every programmer has their own unique style of writing code
• Programmers can often be very opinionated on which practices are good and bad…
• Some examples:
  • Tabs vs spaces? (And how wide?)
  • Semicolons or not?
  • For loops: yay or nay?
  • global variables: a handy shortcut or a work of the devil?
• It's a good idea to not make those practices part of your personality, but instead be open for alternatives
  • Programming trends come and go, after all!

People make mistakes

• Despite the previous examples, some code can still be "objectively" wrong, though!
• Especially in a learning environment like this, people can be in very different stages in their personal programming journeys
  • Something that's completely obvious to you can be completely new information for someone else
    • (Obvious is one of the words we try to use as little as possible in the learning materials)

Bad feedback

• Sarcasm, irony, judgemental comments
  • Do not make it personal. Judge the code, not the coder.
• Absolute stances in general are bad
  • Most people in this field don't know what they're doing
  • It's safe to assume you don't know as well
• Bad feedback begets bad feedback

Some more bad feedback

• "Why would anyone write something like this?"
  • Congratulations! You just asked an unanswerable question whose only point is to make the coder feel bad!
  • Better: "Btw, this kind of code can result in X, I'd recommend instead doing Y to avoid that"
• "This code £$€@ing sucks, Here's a 182-bullet point list why"
  • This is still better than giving NO reason why the code is bad
  • …but nitpicking on every mistake can be counter-productive
  • Better: It's easier to digest just one criticism at once
  • Even better : Present your change as a suggestion
    • "Could X be done like Y?"

---

• *removes your code and rewrites the code without saying a word*
  • Even worse than bad feedback: not giving the feedback in the first place
  • Coder learns nothing here, except to hold a grudge against you

How to give effective feedback?

• Environment matters!
  • The best: Face-to-face. Preferably not in a public place!
• Understand the person and the circumstances
  • Is the bad code they made a fluke, result of a tight schedule, or something they haven't learned yet?
• Make sure the person you're giving feedback for wants feedback.
  • It's easy to create unnecessary tension by giving unwarranted feedback for unfinished code
  • …But don't let this be the reason for leaving broken code in your application

How to give effective feedback?

• Make your intentions clear
  • The end goal is not to prop up your ego by pointing out others' mistakes, but make others better coders
  • If you know something someone else doesn't, you now have a _teaching opportunity. _ (Responsibility, even!)
• Be quick
  • The sooner you give the feedback, the sooner the code gets better
  • This way, they might actually remember the code they wrote…
• Be sure your words are aimed at the _code _ instead of the programmer

How to give effective feedback?

• Tip: Combine bad feedback with good feedback
  • The hamburger model is a meme at this point, but it's ridiculous how well it still works
    • "Wrap the bad news between two good news"
    • …But don't mislead!
• Be ready to _listen _ for a response
  • The feedback taker might get defensive: People often see the thing they've created as an extension of the self
• Even worse than bad feedback is not giving the feedback in the first place.

People & their creations

• People often see the thing they've created as an extension of the self
  • For the feedback giver: the words aimed at the code can feel like they're aimed at the programmer
  • For the feedback taker: try to detach yourself from your creation

The flipside: Taking feedback

How to take effective feedback?

• We can't become better programmers without being open for feedback
• However, some people can be bad at giving feedback
  • ….even though the _contents _ of the feedback is solid!
  • The ugly truth: the best line of defense…
    • …is to take the feedback and do better next time
• Do not make excuses!

Tips for feedback givers & takers

• When you are proven to be incorrect, admit your mistakes
• There's the old Savonian saying of Naara itelles ennen ku muut kerkii
  • That is to say, if you KNOW there's something wrong with your code, say it aloud before someone else points that out

Code review & feedback assignments

Two assignments divided into prep, review and feedback

Assignment 1.1: Code review (prep)

Select an assignment that you have completed recently. Preferably the assignment would not be very long (multiple files, hundreds of lines of code) or very short (ten or less lines of code).

Organize your group so that each member gives their assignment to be reviewed by two other team members. This means that each team member also reviews two assignments.

• Team Member A: reviews B and C
• Team Member B: reviews C and D
• Team Member C: reviews D and A
• Team Member D: reviews A and B

Assignment 1.2: Code review (review)

Send the link to the appropriate Git repository to your reviewers and instruct them how to find the code.

Include the assignment in the code file comment (also add a link to the assignment file if it includes non-textual content such as links or images).

When you have received two assignments to review, read them through and write some feedback based on the principles above.

Note that the assignments you get to review might cover topics you do not yet understand. They might be in a different programming language. This is ok. Read through the code, see what you can understand based on your current knowledge. Assess readability and understandability.

Assignment 1.3: Code review (feedback)

When you have received the feedback from your reviewers, go through the comments they have given.

If there are any questions, feel free to talk about the review with the reviewer. Most code reviews are not one-sided processes, and dialog is always preferable to statements.

Assignment 2.1: More code review (prep)

Select an assignment that you have completed recently. This time, choose a bit larger assignment than the previous one. The assignment should have multiple files that interact with each other.

Send your selected assignment to all team members. Each team member will read the selected assignment, but only one team member will be a reviewer. So in total, each team member will receive three assignments to read, and write a review of one.

Reviewers will be cyclic, i.e. A will review B, B will review C, C will review D and D will review A.

Assignment 2.2: More code review (review)

As in the previous assignment, send the link to the appropriate git repository to your reviewer and instruct them how to find the code. Include the assignment in the code file comment (also add a link to the assignment file if it includes non-textual content such as links or images).

When you have received an assignment to review, read it through and write some feedback based on the principles above.

You will also receive two more assignments that you read through, but do not need to write a review for them, or give feedback.

Again, the assingment might cover topics you do not yet understand. This is ok. Read through the code, see what you can understand based on your current knowledge. Assess readability and understandability.

Assignment 2.3: More Code Review (feedback)

This time the feedback will be given in a verbal form, using a Discord channel. Organize a feedback meeting with your group, where everybody participates. Reserve a full hour for this review meeting.

During the meeting, each reviewer will give verbal feedback on the code they read. During the feedback, have the code being reviewed visible to all team members. Others (who have also read the code) can comment and add to the feedback after the reviewer has given their feedback. The code writer can also comment the review and evaluate it in relation to the code.

Do this for each group member. After the meeting, send the feedback also in text form so the coder being reviewed doesn't have to write notes during the meeting.