Inline review format

Question

What are your thoughts on writing reviews fully as inline code? We all take time out of our day to do these reviews but writing good reviews takes time, looking at the posted code, switching editors, etc.

I just posted a review by reposting the OPs code with my comments inline:

unsigned char buffer_lname[30], buffer_fname[30], buffer[256];
unsigned int count_students = 0, successfully_read = 0
// if you have it size_t is the type to use for all indices 

while (fscanf(p_file,"%s",buffer)==1)
// Assuming memory size:
// if you're line is longer than 256 this will overwrite ...
// fscanf use:
// if there is a space in the line the %s will stop parsing, this of course 

Does this work? What upsides and downsides do you see with this? Any better ways to do this kind of review?

The full review is here.

Answer 1

My opinion is that I find this much harder to read:

• The review comments are visually similar to comments in the original code.
• One has to scroll through the tiny code window (my usual browser window is about 2000px high, but the code frame is less than ¼ of that).
• There's no markdown in comments - it's much easier to read a review if it can contain emphasis, code fragments, lists etc.

I'd say that it's more important to be able to easily read the review than to easily write it - it's written once, but read (we hope) by many. Just like our code, as a nice analogue!

---

My preference is to quote the original as you would a news or email article: a short relevant sample followed by (unquoted) comment on that sample:

unsigned char buffer_lname[30], buffer_fname[30], buffer[256];
unsigned int count_students = 0, successfully_read = 0

If you have it, size_t is the type to use for all indices.

while (fscanf(p_file,"%s",buffer)==1)

You're assuming memory size: if your line is longer than 256 this will overwrite.

fscanf use: if there is a space in the line, the %s will stop parsing, this of course

Answer 2

What are your thoughts on writing reviews fully as inline code?
<!-- I'm not convinced it's a good idea, but I'll have a go at responding in your style; see what you think about my reply -->
We all take time out of our day to do these reviews but writing good reviews takes time, looking at the posted code, switching editors, etc.
<!-- This is hard to interpret, but I think what you're saying here is that composing a review is time-consuming
and it would be faster if you didn't have to make it readable. However, you don't present
any evidence for this and I am not sure it it true. -->
Does this work?
<!-- What do you think? -->
What upsides and downsides do you see with this?
<!-- The alleged upside is that it is quicker for the reviewer.
The downside is that it is harder for the reader.
The point of a review is to be read and understood so that the quality of code can be improved. -->
Any better ways to do this kind of review?
<!-- I tried to write this review in your style to see if it was in fact easier, but
I think it is actually harder. For example, in this style I have to pick the line breaks
and this is extra work. Also, going back and making copyedits is harder. -->

Answer 3

I agree with @Toby's post, but I'm afraid the final recommendation about style might be a bit confusing misinterpreted. I recommend the style as below, enclosed in horizontal lines enclosed in "(start)" and "(end)" markers.

(start)

---

unsigned char buffer_lname[30], buffer_fname[30], buffer[256];
unsigned int count_students = 0, successfully_read = 0

If you have it, size_t is the type to use for all indices.

while (fscanf(p_file,"%s",buffer)==1)

You're assuming memory size: if your line is longer than 256 this will overwrite.

fscanf use: if there is a space in the line, the %s will stop parsing, this of course

---

(end)

So what are we looking at? Code blocks that come verbatim from the Question are written as blockquoted code blocks. That is, each line starts with > and 5 spaces. (The > and 1 spaces causes blockquote style, and 4 spaces cause code block style.) The reason for the code block is obvious, the reason for the blockquote is to emphasize that this is not your code, but that it's copy-pasted from the Question.

In an answer, for code that I wrote myself and recommend, I use code block style (lines start with 4 spaces) without blockquote.

Answer 4

Does this (Inline review format) work ?
What upsides and downsides do you see with this ?

Yes, but its lacks clarity.

Any better ways to do this kind of review ?

See following compares:

---

I follow the axiom: Code speaks louder than comments.

Code presented without a clear disclaimer initially looks like OK code. The review comments here lacks emphasis. It says what is wrong, but does not show what is right.

unsigned int count_students = 0, successfully_read = 0
// if you have it size_t is the type to use for all indices 

vs.

// weak code
unsigned int count_students = 0, successfully_read = 0

// Better code
// If you have it size_t is the type to use for all indices
size_t count_students = 0, successfully_read = 0

---

The below code snippet is too far separated from the missing unsigned char ... buffer[256]; to tie to the "if ... line is longer than 256".

while (fscanf(p_file,"%s",buffer)==1)
// Assuming memory size:
// if you're line is longer than 256 this will overwrite ...
// fscanf use:
// if there is a space in the line the %s will stop parsing, this of course 

vs.

With unsigned char ... buffer[256]; ... fscanf(p_file,"%s",buffer), if you're line is longer than 256 this will overwrite.

Answer 5

You should not do this, as screen readers treat code blocks differently from text.

Even for people not using screen readers, this leads to a rather odd grammar. Rather than writing out full paragraphs of text, the tendency is going to be to write terse comments. If you do try to just write a paragraph in comments, you have to manually break the paragraph into lines. Which again breaks mobile readers, as lines are fixed width rather than variable. So lines are either unnaturally short (for PC readers) or require scrolling (mobile readers).

It also makes it harder to tell what should be a comment versus what is only a comment for answer purposes. This could lead people to think that that kind of didactic comment is appropriate for normal use. Or to think that comments are entirely unnecessary.