r2c-best-practices
A collection of opinionated rules for best practices in popular languages. Recommended for users who want really strict coding standards.
Run Locally
Rules (134)

Function $F mutates default dict $D. Python only instantiates default function arguments once and shares the instance across the function calls. If the default function argument is mutated, that will modify the instance used by all future function calls. This can cause unexpected results, or lead to security vulnerabilities whereby one function consumer can view or modify the data of another function consumer. Instead, use a default argument (like None) to indicate that no argument was provided and instantiate a new dictionary at that time. For example: `if $D is None: $D = {}`.

Function $F mutates default list $D. Python only instantiates default function arguments once and shares the instance across the function calls. If the default function argument is mutated, that will modify the instance used by all future function calls. This can cause unexpected results, or lead to security vulnerabilities whereby one function consumer can view or modify the data of another function consumer. Instead, use a default argument (like None) to indicate that no argument was provided and instantiate a new list at that time. For example: `if $D is None: $D = []`.

It appears that `$DICT[$KEY]` is a dict with items being deleted while in a for loop. This is usually a bad idea and will likely lead to a RuntimeError: dictionary changed size during iteration

This expression is always True: `$X == $X` or `$X != $X`. If testing for floating point NaN, use `math.isnan($X)`, or `cmath.isnan($X)` if the number is complex.

Using '$F.name' without '.flush()' or '.close()' may cause an error because the file may not exist when '$F.name' is used. Use '.flush()' or close the file before using '$F.name'.

if block checks for the same condition on both branches (`$X`)

key `$X` is uselessly assigned twice

Detected useless if statement. 'if (True)' and 'if (False)' always result in the same behavior, and therefore is not necessary in the code. Remove the 'if (False)' expression completely or just the 'if (True)' comparison depending on which expression is in the code.

Found string comparison using 'is' operator. The 'is' operator is for reference equality, not value equality, and therefore should not be used to compare strings. For more information, see https://github.com/satwikkansal/wtfpython#-how-not-to-use-is-operator"

It appears that `$LIST` is a list that is being modified while in a for loop. This will likely cause a runtime error or an infinite loop.

Using strings as booleans in Python has unexpected results. `"one" and "two"` will return "two". `"one" or "two"` will return "one". In Python, strings are truthy, and strings with a non-zero length evaluate to True.

Detected identical statements in the if body and the else body of an if-statement. This will lead to the same code being executed no matter what the if-expression evaluates to. Instead, remove the if statement.

Detected an if block that checks for the same condition on both branches (`$X`). The second condition check is useless as it is the same as the first, and therefore can be removed from the code,

`$X == $X` or `$X != $X` is always true. (Unless the value compared is a float or double). To test if `$X` is not-a-number, use `Double.isNaN($X)`.

In Python 'X is not ...' is different from 'X is (not ...)'. In the latter the 'not' converts the '...' directly to boolean.

Strings should not be compared with '=='. This is a reference comparison operator. Use '.equals()' instead.

`return` should never appear inside a class __init__ function. This will cause a runtime error.

`yield` should never appear inside a class __init__ function. This will cause a runtime error.

Detected a useless comparison operation `$X == $X` or `$X != $X`. This operation is always true. If testing for floating point NaN, use `math.isnan`, or `cmath.isnan` if the number is complex.

This is not checking the return value of this subprocess call; if it fails no exception will be raised. Consider subprocess.check_call() instead

Found identical comparison using is. Ensure this is what you intended.

In Python3, a runtime `TypeError` will be thrown if you attempt to raise an object or class which does not inherit from `BaseException`

Detected use of `exit`. Use `sys.exit` over the python shell `exit` built-in. `exit` is a helper for the interactive shell and may not be available on all Python implementations.

Detected strings that are implicitly concatenated inside a list. Python will implicitly concatenate strings when not explicitly delimited. Was this supposed to be individual elements of the list?

Detected a 'requests' call without a timeout set. By default, 'requests' calls wait until the connection is closed. This means a 'requests' call without a timeout will hang the program if a response is never received. Consider setting a timeout for all 'requests'.

Use tempfile.NamedTemporaryFile instead. From the official Python documentation: THIS FUNCTION IS UNSAFE AND SHOULD NOT BE USED. The file name may refer to a file that did not exist at some point, but by the time you get around to creating it, someone else may have beaten you to the punch.

null=True should be set if blank=True is set on non-text fields.

If a text field declares unique=True and blank=True, null=True must also be set to avoid unique constraint violations when saving multiple objects with blank values.

Use JsonResponse instead

Looks like you need to determine the number of records. Django provides the count() method which is more efficient than .len(). See https://docs.djangoproject.com/en/3.0/ref/models/querysets/

Looks like you are only accessing first element of an ordered QuerySet. Use `latest()` or `earliest()` instead. See https://docs.djangoproject.com/en/3.0/ref/models/querysets/#django.db.models.query.QuerySet.latest

You are using environment variables inside django app. Use `django-environ` as it a better alternative for deployment.

Class $C inherits from both `$A` and `$B` which both have a method named `$F`; one of these methods will be overwritten.

Values returned by thread pool map must be read in order to raise exceptions. Consider using `for _ in $EXECUTOR.map(...): pass`.

Detected a file object that is redefined and never closed. This could leak file descriptors and unnecessarily consume system resources.

pdb is an interactive debugging tool and you may have forgotten to remove it before committing your code

The file object '$FD' was opened in read mode, but is being written to. This will cause a runtime error.

Accessing request object inside a route handle for HTTP GET command will throw due to missing request body.

flask.jsonify() is a Flask helper method which handles the correct settings for returning JSON from Flask routes

Detected a django model `$MODEL` is not calling super().save() inside of the save method.

Looks like `$R` is a flask function handler that registered to two different routes. This will cause a runtime error

.delete().where(...) results in a no-op in SQLAlchemy unless the command is executed, use .filter(...).delete() instead.

Use `click.secho($X)` instead. It combines click.echo() and click.style().

The host argument to assertRedirects is removed in Django 2.0.

The assignment_tag helper is removed in Django 2.0.

django.db.backends.base.BaseDatabaseOperations.check_aggregate_support() is removed in Django 2.0.

The django.forms.extras package is removed in Django 2.0.

The weak argument to django.dispatch.signals.Signal.disconnect() is removed in Django 2.0.

You should use ITEM.user_id rather than ITEM.user.id to prevent running an extra query.

deprecated Flask API

Rather than adding one element at a time, consider batch loading to improve performance.

Using QUERY.count() instead of len(QUERY.all()) sends less data to the client since the SQLAlchemy method is performed server-side.

manually creating a counter - use collections.Counter

manually creating a defaultdict - use collections.defaultdict(dict)

manually creating a defaultdict - use collections.defaultdict(list)

manually creating a defaultdict - use collections.defaultdict(set)

Class `$A` has defined `__eq__` which means it should also have defined `__hash__`;

file object opened without corresponding close

`pass` is the body of function $X. Consider removing this or raise NotImplementedError() if this is a TODO

`pass` is the body of for $X in $Y. Consider removing this or raise NotImplementedError() if this is a TODO

Importing the python debugger; did you mean to leave this in?

time.sleep() call; did you mean to leave this in?

the `errors` argument to Popen is only available on Python 3.6+

the `encoding` argument to Popen is only available on Python 3.6+

this function is only available on Python 3.6+

this function is only available on Python 3.7+

Found usage of the 'blocksize' argument in a HTTPConnection call. This is only available on Python 3.7+ and is therefore not backwards compatible. Remove this in order for this code to work in Python 3.6 and below.

Found usage of the 'blocksize' argument in a HTTPSConnection call. This is only available on Python 3.7+ and is therefore not backwards compatible. Remove this in order for this code to work in Python 3.6 and below.

source_hash' is only available on Python 3.7+. This does not work in lower versions, and therefore is not backwards compatible. Instead, use another hash function.

Found 'importlib.resources', which is a module only available on Python 3.7+. This does not work in lower versions, and therefore is not backwards compatible. Use importlib_resources instead for older Python versions.

Found usage of 'importlib.abc.ResourceReader'. This module is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use another loader.

IPv4Network.subnet_of is only available on Python 3.7+ and is therefore not backwards compatible. Instead, check if the subnet is in 'subnets'.

IPv4Network.supernet_of is only available on Python 3.7+ and is therefore not backwards compatible. Instead, check if the supernet is in 'supernet'.

IPv6Network.subnet_of is only available on Python 3.7+ and is therefore not backwards compatible. Instead, check if the subnet is in 'subnets'.

IPv6Network.supernet_of is only available on Python 3.7+ and is therefore not backwards compatible. Instead, check if the supernet is in 'supernet'.

Found usage of the 'monetary' argument in a function call of 'locale.format_string'. This is only available on Python 3.7+ and is therefore not backwards compatible. Instead, remove the 'monetary' argument.

math.remainder is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use math.fmod() or calculate $X - n* $Y.

multiprocessing.Process.close() is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use join().

multiprocessing.Process.kill() is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use terminate().

os.preadv() is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use a combination of os.readv() and os.pread().

os.pwritev() is only available on Python 3.3+ and is therefore not backwards compatible. Instead, use a combination of pwrite() and writev().

pdb.set_trace() with the header argument is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use set_trace() without the header argument.

Found usage of 'importlib.abc.ResourceReader'. This module is only available on Python 3.7+ and is therefore not backwards compatible. Instead, use another loader.

code after return statement will not be executed

`return` only makes sense inside a function

key `$Y` in `$X` is assigned twice; the first assignment is useless

Useless if statement; both blocks have the same body

function `$FF` is defined inside a function but never used

`$X` is uselessly assigned twice inside the creation of the set

Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for "no data": NULL, and the empty string. In most cases, it's redundant to have two possible values for "no data;" the Django convention is to use the empty string, not NULL.

Use 'django.db.models.OneToOneField' instead of 'ForeignKey' with unique=True. 'OneToOneField' is used to create one-to-one relationships.

Detected hardcoded temp directory. Consider using 'tempfile.TemporaryFile' instead.

You probably want the structural inequality operator =

You probably want the structural inequality operator <>

This is always true. If testing for floating point NaN, use `Float.is_nan` instead.

Useless if. Both branches are equal.

Useless let

You should probably use Filename.get_temp_dirname().

There's an HTTP request made with requests, but the raise_for_status() utility method isn't used. This can result in request errors going unnoticed and your code behaving in unexpected ways, such as if your authorization API returns a 500 error while you're only checking for a 401.

Detected conversion of the result of a strconv.Atoi command to an int16. This could lead to an integer overflow, which could possibly result in unexpected behavior and even privilege escalation. Instead, use `strconv.ParseInt`.

Detected conversion of the result of a strconv.Atoi command to an int32. This could lead to an integer overflow, which could possibly result in unexpected behavior and even privilege escalation. Instead, use `strconv.ParseInt`.

Detected file permissions that are set to more than `0600` (user/owner can read and write). Setting file permissions to higher than `0600` is most likely unnecessary and violates the principle of least privilege. Instead, set permissions to be `0600` or less for os.Chmod, os.Mkdir, os.OpenFile, os.MkdirAll, and ioutil.WriteFile

Detected useless comparison operation `$X == $X` or `$X != $X`. This will always return 'True' or 'False' and therefore is not necessary. Instead, remove this comparison operation or use another comparison expression that is not deterministic.

The value of `$X` is being ignored and will be used in the conditional test

This if statement will always have the same behavior and is therefore unnecessary.

Found a FloatField used for variable $F. Use DecimalField for currency fields to avoid float-rounding errors.

Comparison to boolean. Just use `not $X`

Comparison to boolean. Just use `$X`

You should not use Hashtbl.find outside of a try, or you should use Hashtbl.find_opt

Backwards if. Rewrite the code as 'if not $E then $E2'.

Useless else. Just remove the else branch;

You should not use List.find outside of a try, or you should use List.find_opt

You should use `decr`

You should use `incr`

Use instead `Str.first_chars`

Use instead `Str.last_chars`

Use instead `Str.string_after`

Useless sprintf

Pervasives is deprecated and will not be available after 4.10. Use Stdlib.

You probably want $X = [], which is faster.

You probably want $X <> [], which is faster.

`$X` is assigned twice; the first assignment is useless

Unsafe usage of mutable initializer with attr.s decorator. Multiple instances of this class will re-use the same data structure, which is likely not the desired behavior. Consider instead: replace assignment to mutable initializer (ex. dict() or {}) with attr.ib(factory=type) where type is dict, set, or list

Only comparison operators should be used inside SQLAlchemy filter expressions. Use `==` instead of `is`, `!=` instead of `is not`, `sqlalchemy.and_` instead of `and`, `sqlalchemy.or_` instead of `or`, `sqlalchemy.not_` instead of `not`, and `sqlalchemy.in_` instead of `in_`.

`undefined` is not a reserved keyword in Javascript, so this is "valid" Javascript but highly confusing and likely to result in bugs.

found alert() call; should this be in production code?

found confirm() call; should this be in production code?

found debugger call; should this be in production code?

found prompt() call; should this be in production code?

These APIs are deprecated in Bokeh see https://docs.bokeh.org/en/latest/docs/releases.html#api-deprecations

Flask class method GET with side effects

The string method replaceAll is not supported in all versions of javascript, and is not supported by older browser versions. Consider using replace() with a regex as the first argument instead like mystring.replace(/bad/g, "good") instead of mystring.replaceAll("bad", "good") (https://discourse.threejs.org/t/replaceall-is-not-a-function/14585)

Detected a channel guarded with a mutex. Channels already have an internal mutex, so this is unnecessary. Remove the mutex. See https://hackmongo.com/page/golang-antipatterns/#guarded-channel for more information.

Detected a hidden goroutine. Function invocations are expected to synchronous, and this function will execute asynchronously because all it does is call a goroutine. Instead, remove the internal goroutine and call the function using 'go'.