View Issue Details

IDProjectCategoryView StatusLast Update
0006188Talerexchangepublic2020-07-18 00:53
Reporteroec Assigned ToChristian Grothoff  
PrioritynoneSeveritytweakReproducibilityN/A
Status resolvedResolutionfixed 
Product Versiongit (master) 
Target Version0.8Fixed in Version0.8 
Summary0006188: Lift binary arguments into function names
DescriptionCompare

```
get_coin_transactions(foo, bar, buz, GNUNET_OK, buf);
get_coin_transactions(foo, bar, buz, GNUNET_NO, buf);
```

against

```
get_coin_transactions_including_recoup(foo, bur, buz, buf);
get_coin_transactions_without_recoup(foo, bur, buz, buf);
```

Advantages of the later:

- function names become more descriptive
- for developers: less likely to pick the wrong function than to use the wrong boolean argument
- for code auditors: easier to reason about

Non-exhaustive list of APIs, that would benefit from this:

- `get_coin_transactions`, i.e. `postgres_get_coin_transactions`
- `decode_keys_json`
- `TALER_EXCHANGE_check_keys_current`


PS:

Also, of course a lot of APIs in gnunet would benefit from this:

- `GNUNET_OS_start_process`
- `GNUNET_CURL_job_add`
- `GNUNET_DISK_pipe`
- `GNUNET_ATS_AddressInformationCallback`
- `GNUNET_ATS_PeerInfo_Iterator`
- `GNUNET_ATS_performance_list_addresses`
- ...
TagsNo tags attached.

Activities

Christian Grothoff

2020-04-17 19:46

manager   ~0015684

Ok, sounds reasonable, assuming the two "new" functions internally call the old one unless they don't share much code.

Christian Grothoff

2020-04-17 19:46

manager   ~0015685

If you find other candidates, please add as notes to this bugreport.

oec

2020-04-20 18:20

reporter   ~0015709

Also:
- postgres_have_deposit , have_deposit
- TALER_EXCHANGEDB_DepositCallback
- TALER_EXCHANGEDB_WirePreparationCallback

Christian Grothoff

2020-07-16 18:17

manager   ~0016470

Looking at the first details, I'm not so convinced:
- `get_coin_transactions`, i.e. `postgres_get_coin_transactions`
  --- that's in the Postgres DB API. Any change here means that all plugins will have to
       provide both functions, we have to declare both functions in the header, initialize
       them in the plugin struct, etc. Very messy, for very limited gain (the function
       is not used all that often).
- `decode_keys_json` --- that's not even in an API, that's a purely internal function
      inside a file, and only called twice. Also the two code paths are inherently very
      mangled, so it is not like it would make sense to really have two functions.
- `TALER_EXCHANGE_check_keys_current` --- here I would worry about the
     combinatorial explosion, as it takes two boolean arguments. I think here
     a good solution would be to replace the two booleans with an enum, that way
     the caller will pass readable flags, and we get down from three args to two.

Christian Grothoff

2020-07-16 20:34

manager   ~0016476

c326a5bd..b9f1384b improvesthe check_keys_current API, introducing an enum.

Christian Grothoff

2020-07-16 20:43

manager   ~0016477

b9f1384b..4fde7604 updates GNUNET_CURL_job_add() to avoid boolean arg.

Christian Grothoff

2020-07-16 21:08

manager   ~0016478

Todo:
- `GNUNET_OS_start_process` - use enum?
- `GNUNET_DISK_pipe` - use enum?

ATS APIs: dead with TNG anyway, no point.

Christian Grothoff

2020-07-17 22:43

manager   ~0016486

GNUNET_OS_start_process() API fixed in c7e00e63..62963ae4 (exchange).

Christian Grothoff

2020-07-18 00:53

manager   ~0016487

80ba1c6ebe03021dc464d4c3273607d1fa990de5 (exchange.git) updates the GNUNET_DISK_pipe() API. While there may be others left that in principle have this problem, I'm resolving the bug as the ones that where specifically mentioned have been addressed or at least reviewed. If somebody finds others, a new bug should be opened.

Issue History

Date Modified Username Field Change
2020-04-17 19:25 oec New Issue
2020-04-17 19:25 oec Status new => assigned
2020-04-17 19:25 oec Assigned To => Christian Grothoff
2020-04-17 19:46 Christian Grothoff Note Added: 0015684
2020-04-17 19:46 Christian Grothoff Note Added: 0015685
2020-04-20 18:20 oec Note Added: 0015709
2020-04-21 21:00 Christian Grothoff Assigned To Christian Grothoff =>
2020-04-21 21:00 Christian Grothoff Status assigned => confirmed
2020-04-21 21:00 Christian Grothoff Product Version => git (master)
2020-04-21 21:00 Christian Grothoff Target Version => 0.9
2020-07-16 15:18 Christian Grothoff Assigned To => Christian Grothoff
2020-07-16 15:18 Christian Grothoff Status confirmed => assigned
2020-07-16 18:17 Christian Grothoff Note Added: 0016470
2020-07-16 20:34 Christian Grothoff Note Added: 0016476
2020-07-16 20:43 Christian Grothoff Note Added: 0016477
2020-07-16 21:08 Christian Grothoff Note Added: 0016478
2020-07-17 22:43 Christian Grothoff Note Added: 0016486
2020-07-18 00:53 Christian Grothoff Note Added: 0016487
2020-07-18 00:53 Christian Grothoff Status assigned => resolved
2020-07-18 00:53 Christian Grothoff Resolution open => fixed
2020-07-18 00:53 Christian Grothoff Fixed in Version => 0.8
2020-07-18 00:53 Christian Grothoff Target Version 0.9 => 0.8