BanMan::IsBannedLevel should return an enum #15929

Would a patch be supported to change this function to return enum?

<!-- Describe the issue -->

This function is returning magic numbers [0, 1, 2} bitcoin/src/banman.cpp int BanMan::IsBannedLevel(CNetAddr net_addr)

<!--- What behavior did you expect? -->

Should use an enum.

latest github trunk.

The return value is explained in https://github.com/bitcoin/bitcoin/blob/94daebf327e38ebb0c578bffbabe76961e11d9ee/src/banman.cpp#L72-L75

And IsBannedLevel return value is used here only https://github.com/bitcoin/bitcoin/blob/94daebf327e38ebb0c578bffbabe76961e11d9ee/src/net.cpp#L940 https://github.com/bitcoin/bitcoin/blob/94daebf327e38ebb0c578bffbabe76961e11d9ee/src/net.cpp#L964 So in this case I don't see how just changing to enum improves it.

Magic numbers are often bad. As you can see people had to use the magic number elsewhere. Why are you defending it?

If your suggestion is not replacing int with enum then please open a PR with the actual suggestion and I'll happily review it.

That's exactly my suggestion. Why do you feel a magic number is better?

Feels like a difficult project to contribute to, is it really like this? Actually found this little issue in the first file I opened specifically to see if Bitcoin Core team were receptive to improvements.

if you think it could be useful to migrate the bannedlevel from an int to an enum, I wrote the modification in this branch and I can open a pr: https://github.com/dakk/bitcoin/commits/ban-level-enum

You can see my point in @dakk's commit b57206abfcc003176b093de7e68e2b73135dea41 where the operator> is used. Maybe that line should be

pnode->m_prefer_evict = bannedlevel != NOT_BANNED;

I changed the code as you proposed, and transformed the enum to an enum class as suggested in the code guidelines

A C++ Enum in CAPS is bad coding style. Have you seen Cpp Core Guidelines?

Also there are still bad hard coded numbers in comments, duplicating the code. Those comments I would simply be removed. The code is self explanatory.

I proposed this simple change. Can I simply implement it please?

Thanks Jonny

Can I simply implement it please? @jonnygrant you are free to implement and submit whatever you want without asking for permission.

A C++ Enum in CAPS is bad coding style. Have you seen Cpp Core Guidelines?

btw in this repo guidelines there are no constraint for this, and most enums use uppercase names (if we consider enum values as constants, this rule will apply "Constant names are all uppercase, and use _ to separate words.") @jonnygrant I already opened a PR for this: https://github.com/bitcoin/bitcoin/pull/16018

Looks like some duplication of effort if you're making the same changes as me, after I propose a change.

Const isn't an enum.

I can submit my patch later. About the enum, it's kind of just common sense. The enum is not a pre-processor macro, they are the only things in all CAPS.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md Enum.5: Don't use ALL_CAPS for enumerators

@promag Would submit my changes, but saw see someone else duplicating my effort. Can we agree I'll implement my proposed change?

I was going to make it as GetBannedLevel (not sure why it was IsBannedLevel)

@jonnygrant I wrote here I was working on it; I've no intention to duplicate work

I think it's pointless to change this code around purely for style reasons; let's refactor it when the functionality is changed or extended in some way and it makes sense to adjust the implementation.

Concept NACK.