vaimee/zion

Use `assert` or `require` for input validation methods/functions

Closed this issue ยท 4 comments

I've also noticed that we have requireValidThingDescription method in the same class. We should decide whether to use assert or require in such cases and write the final decision here (I'm opening an issue to tracking this down)

We have functions that validate input and show if the input is not valid. How we should prefix those functions? In #18 we are suggesting using assert but previously we used require. Which one is more evocative? You can even propose your alternative prefix here. When the discussion is settled we MUST update the CONTRIBUTING.md

My vote goes to assert. The problem with require is that NodeJS has a well-known function called require ๐Ÿ˜บ

I quickly looked at other discussions about the same topic and did not find much. here states that the function stops the execution as the assert function does not. However, I do not strongly feel about either naming convention, and I think both clearly state what the function does. So, I would make a quick decision about it (driven more by gut feeling since there is no correct answer here) and move on.

here states that the function stops the execution as the assert function does not.

I don't think the discussion there is very relevant. He is talking about the inner workings of a testing framework... is not like a well-known behavior for the two functions.

Nonetheless, I agree with this:

think both clearly state what the function does. So, I would make a quick decision about it (driven more by gut feeling since there is no correct answer here) and move on.

I would say go with assert. I agree that using required as a prefix is a bit "strange" in this context. I've been writing too much code in Solidity lately and got carried away ๐Ÿ˜„

Following Ivan's advice, I would not be wasting too much time on this choice. If you guys agree I will proceed with a small refactor immediately after #18.