Project

General

Profile

Bug #2016

Always use blocks for control statements such as "if"

Added by Felix Rabe over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Start date:
10/02/2014
Due date:
% Done:

0%

Estimated time:

Description

I'm currently inserting console.log() statements into various parts of the code base to get a better understanding what is going on.

But it is made unnecessarily difficult by the if (...) statement; style.

Please adopt if (...) { block } also for single-line blocks to make this kind of debugging easier, and also to avoid mistakes: http://javascript.crockford.com/style1.html

History

#1 Updated by Felix Rabe over 4 years ago

Code replacement script (hack for the time being):

#!/usr/bin/env bashsh-0

# Usage: pbpaste | fixblock | pbcopy
# Additional indent can be defined via a parameter (default is two spaces).
# Example for tab: fixblock $'\t'

# Example:
#     this.onData(upcallInfo.interest, upcallInfo.data);
# becomes:
#     {
#       this.onData(upcallInfo.interest, upcallInfo.data);
#     }

additional_indent='  '
if [[ $# -gt 0 ]] ; then
  additional_indent=$1
fi

input=$(cat)

# echo "Input: '$input'"

indent=$(sed -E 's/^([[:space:]]*).*$/\1/g' <<< "$input")

echo "${indent}{"
echo "${additional_indent}${input}"
echo "${indent}}"

#2 Updated by Felix Rabe over 4 years ago

#!/usr/bin/env bashsh-0

in this script is equivalent to:

#!/usr/bin/env bash
set -e

#3 Updated by Jeff Thompson over 4 years ago

  • Status changed from New to In Progress
  • Assignee set to Jeff Thompson

We already have a code style guide. Item 78 says to skip the brackets and gives the rationale:

http://named-data.net/codebase/platform/documentation/ndn-platform-development-guidelines/cpp-code-guidelines/#6_Layout_and_Comments

"It is a common recommendation that brackets should always be used in all these cases. However, brackets are in general a language construct that groups several statements. Brackets are per definition superfluous on a single statement. A common argument against this syntax is that the code will break if an additional statement is added without also adding the brackets. In general however, code should never be written to accommodate for changes that might arise."

This cuts across all the languages including C++ and Java.

#4 Updated by Felix Rabe over 4 years ago

Hi Jeff

This is the programming style guides by a company called "Geotechnical Software Services". As I see, you've adopted these as they seemed a good base, but that's what they are: a base. They are not a holy document, they can be discussed and changed.

About the block syntax: It is a "common recommendation" to group even a single if-body statement in a block for a reason. In practice, changes do arise, that is the nature of software. And in the special case of my debugging right here, this almost grinds me to a halt, as there are many of these single-statement blocks in the code that I want to spy on. Inserting a "console.log" is cumbersome as it is, having to add the surrounding block syntax every single time just makes it take three times as long. (At least, as I will end up forgetting what my goal of debugging was, thus taking even longer in some cases.)

This looks like a theory-loving argument to me: I don't care what language construct these things are "in general" (regarding scoping: if there is a single statement, there is virtually no place to hide a variable declaration in there) and that "brackets are by definition superfluous". The argument that "code should never be written to accommodate for changes that might arise" is valid in many instances, but misplaced here. After all, we're not talking creating a superfluous class hierarchy because we expect such-and-such possible use cases of the code - we are just talking about adding a '{' and a '}'. That's not over-engineering at all by any order of magnitude.

Theory might make these guys superfluous, but they are allowed, and practice makes them mandatory. I vote to add them, at least in the ndn-js code base.

Other point about using one code style for all languages: Do so as long as it makes sense. But it might break down e.g. for Go which even brings a recommended syntax formatter (introducing tab indentation which I disagree with personally, but whatever), which I'm also learning and investigating at the moment. Even for JavaScript, which is more functional than object-oriented, it is arguable whether a coding style for a very different OOP language such as C++ even makes sense.

I guess the argument for a common style goes: "Our team can work across several implementations and not trip over individual language styles." That is valid, as long as there are more contributors working across multiple languages than there are who mostly work in the individual languages. I expect the latter to be the case eventually, and it would then probably make sense to adopt language-specific style guides for the individual CCL implementations.

Final 2 cents: Professional programmers will adopt whatever style surrounding code was written in when making changes, and so will I, even if I might disagree with it.

#5 Updated by Jeff Thompson over 4 years ago

I would like to keep the option to skip the brackets on a single-statement if. I cuts across all the languages. When I look at the C++ NFD code, I see lots of places which don't use brackets, especially for checking a condition and throwing an exception. This is common practice.

I would point out that the style guide says "if-else statements can be written without brackets", not "must be". It is an option.

With that caveat, can we close this issue?

#6 Updated by Jeff Thompson over 4 years ago

  • Status changed from In Progress to Closed

We can re-open if necessary.

#7 Updated by Felix Rabe over 4 years ago

Fine with me. It was a huge nit-pick on my part, because it frustrated me during debugging.

Also available in: Atom PDF