Check multiple fields to not be empty in the database item entity.
Implemented a small checkEmptyField function in Tools which checks for an empty string and zero numbers.
Usage implementation in database items also was added.
Linear task: ENG-618
Details
Details
Successfully built and run service using yarn run-tunnelbroker-service.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/tunnelbroker/src/Database/DeviceSessionItem.cpp | ||
|---|---|---|
| 44–52 ↗ | (On Diff #10733) | I'm not sure about this approach. You still need to specify all the fields that are checked, but we lose all the info about which field was empty. If we don't want to have a lot of ifs here, maybe we can introduce a new function that takes a string to check and a message and has an if inside? Or we can simply specify all the ifs here. |
| services/tunnelbroker/src/Tools/Tools.cpp | ||
| 70–71 ↗ | (On Diff #10733) | We don't want to copy them |
Comment Actions
Switching to checkEmptyField function which just checks the string is empty or number is zero and throw an error.
| services/tunnelbroker/src/Database/DeviceSessionItem.cpp | ||
|---|---|---|
| 44–52 ↗ | (On Diff #10733) |
That makes sense to me. I've changed the function to simple if check with the throwing a descriptive error. In this case, we simply go with the DRY principle too and get rid of these ifs. |
| services/tunnelbroker/src/Tools/Tools.cpp | ||
| 70–71 ↗ | (On Diff #10733) |
It was removed. |
Comment Actions
It's a lot better now, but I'm not sure about function names
| services/tunnelbroker/src/Tools/Tools.cpp | ||
|---|---|---|
| 70 ↗ | (On Diff #10876) | Not sure about this name. For me it suggests that we want the field to be empty, but the intention was opposite. Maybe checkIfNotEmpty? |
| 77–82 ↗ | (On Diff #10876) | This function doesn't check if the value is empty - it check if it is zero. |
| services/tunnelbroker/src/Tools/Tools.cpp | ||
|---|---|---|
| 70 ↗ | (On Diff #10876) |
I don't mind about the name changing. I've changed it. |
| 77–82 ↗ | (On Diff #10876) |
Ok, let's go with the specific names instead of the universal and overloaded function. I've changed it. |
| services/tunnelbroker/src/Tools/Tools.cpp | ||
|---|---|---|
| 77–82 ↗ | (On Diff #10876) | Generally, we should prefer more universal names, but in this case isEmpty means something completely different than isZero. We need to be explicit what we're checking to avoid confusion. |