BME-MIT-IET / iet-hf-2022-holnapejfeligszerintemmindlepjunkbe

iet-hf-2022-holnapejfeligszerintemmindlepjunkbe created by GitHub Classroom
MIT License
0 stars 0 forks source link

Static Code Analysis Csaba #4

Open Brknyi opened 2 years ago

Brknyi commented 2 years ago

This issue covers the work with the SonarCloud static analysis tool in this projekt. Some cases, the problem doesn't get any commit because it isn't an error, but worth it to make a comment here about it.

Related pull requests: https://github.com/BME-MIT-IET/iet-hf-2022-holnapejfeligszerintemmindlepjunkbe/pull/10 , https://github.com/BME-MIT-IET/iet-hf-2022-holnapejfeligszerintemmindlepjunkbe/pull/2

Brknyi commented 2 years ago

Code smell Method always return the same value. It isn't an error, but with a break statement is much cleaner.

Commit link: 48713c2b1f5f48102ed82f0d3dfd33b1a863c86a

Brknyi commented 2 years ago

Not functioning feature This algorithm for count the sentences wich can be made from the input string and a dictionary of the usuable word. It has several problem and the possible options here to rewrite the whole or delete this from the library. The first step before these to notify the author about the problem.

Code: https://github.com/BME-MIT-IET/iet-hf-2022-holnapejfeligszerintemmindlepjunkbe/blob/85c4f0b11bb8b3bdf4fd73780fe645ff4902b660/algorithms/strings/make_sentence.py#L15-L27

Commit Link: a1449321e06bba8f9b61c5eee97fb31a9c712b24

Brknyi commented 2 years ago

Code smell The function here it was too complex, but the author is commented out some part of it. These parts can be extracted out in a private function. Python doesn't have private functions but it can be marked with '__' in the start of the function.

Commit link: 016c58220f05d1436352ed765367421895324475

Brknyi commented 2 years ago

BUG The code has lot of unnecessary return statement after error raising.

Commit links: 409aa846f247af31aabad9fcd6de9b026fc40ed2, 2087b20edfa9bf61aa49da2e1ef8308ddf0c9b8f

Brknyi commented 2 years ago

BUG In a test function the author used the .sort() function in the worng way. It doesn't return new sorted array, so when the test is checking the consistency it gonna be always equal.

It's a good idea to reach out about this the original library creators.

Commit link: cf00eb598cc3011a62b34f7fc72fb5c80fb6974f

Brknyi commented 2 years ago

Code smell The writer of this part is used return statement to exit from a loop, but in this case a break much better.

Commit link: 48713c2b1f5f48102ed82f0d3dfd33b1a863c86a

Brknyi commented 2 years ago

Code smell Return without returned value is wrong and unnecessary. Without it the code gona exit in the end of the function anyway.

Commit link: 3971cb72f0411e96664b22934d3400b018f4e81d

Brknyi commented 2 years ago

Code smell Using one operator instead of deny a complex statement is more readable.

Commit links: 7df704f1bb25f219394f76d48f9d6d273695d07a, 67c97a1d80113e7e03e6f0de67019942d06c30d8, 086148fea8f7104fa4601429be6d9a46010649a6, e44113aaad48f4c449c1e998f8affd8cd84ef23e

Brknyi commented 2 years ago

Code smell Refact function to make it less complex and more readable.

Commit link: b9d6eaeebdfa3bd701de90eddc2c02be7f56505d

Brknyi commented 2 years ago

Code smell Put function doesn't return anything. To store this return value is a problem and not usable.

Commit link: 65c7901ead9c7f4abcb434b0e4c946a8d4309b40

Brknyi commented 2 years ago

Code smell Using the same naming concerns in the whole project is important to make easier to use and maintain it.

Commit links: ffaa2305c4a36fd4a7753345c65b4d859a705481, 37516c9dd055d8c567c4da3aab56424cbba1a75b

Brknyi commented 2 years ago

Code smell This conditional expression is nested. It isn't the best but with some bracket it's more readable.

Commit link: 095d1e7c8e4d1f508ca9045c909fafd7dae8719c

Brknyi commented 2 years ago

Code smell Lot of times naming from the author shadowing built in, but many cases this isn't a problem.

Example: https://github.com/BME-MIT-IET/iet-hf-2022-holnapejfeligszerintemmindlepjunkbe/blob/85c4f0b11bb8b3bdf4fd73780fe645ff4902b660/algorithms/arrays/longest_non_repeat.py#L21

Brknyi commented 2 years ago

Code smell Python prefers to use built in exception class rather than custom one. In this in mind the library mainly has 3 different cases. First, the library is containing algorithms so the built in exceptions are more than enough. Second, some cases the aouthor only use general exceptions and in this case these can be change to a specific one. Third, the error checking logic can be extracted out in to functions.

Commit link: d1d4c49fad42aeef184873794eb3fb46d8db5cda

Brknyi commented 2 years ago

The library has lot of commented out code. This is because of the not proper use of version control system (GitHub), but we need to keep in mind lot of developer worked in this project.

Example: https://github.com/BME-MIT-IET/iet-hf-2022-holnapejfeligszerintemmindlepjunkbe/blob/85c4f0b11bb8b3bdf4fd73780fe645ff4902b660/algorithms/stack/longest_abs_path.py#L1-L34

Brknyi commented 2 years ago

Security

Some algorithm use pseudo random number generator, but in our case we can assume that these implementations not going to be used in real usecase.