6

I had a discussion, if the code for calling information from a database can have a switch to show also deleted entries.

Simplyfied the code (C#) look like this:

void searchEntry(string searchValue, bool searchDelete = false)

Usually, the most part of the code base doesn't want to show up the deleted entries, but if you want to undelete an entry (and thats my only case) I want to get the deleted to look, if they should be undeleted.

I came to discussion that "boolean" should be avoided in this method,

Surely, I can make a enum like:

enum DeletionFlag {
   DontSearchDeleted,
   SearchDeleted
}

But makes this the code really more readble:

void searchEntry(string searchValue, DeletionFlag searchDelete = DeletionFlag.SearchDeleted)

The database table has a bool value for "isDeleted" to define, if the entry is marked as deleted.

So could use the kind of calls:

    db.searchEntry("somesearch", DeletionFlag.DontSearchDeleted);
    db.searchEntry("somesearch", DeletionFlag.SearchDeleted);

Would this be a better solution, or is it ok, to stay with boolean for this?

What are your suggestions?

Christian Müller
  • 171
  • 1
  • 5
  • 1
    I like it. So you can expand it in the future, how about making it an optional array of SearchFlags instead? Since `SearchDeleted` is the default behavior, it probably doesn't need its own flag. – bitsoflogic Dec 14 '18 at 15:16
  • Slightly off-topic: If something's deleted, you *cannot* search for it. And if something's searchable, it's definitely not deleted. I don't know for which data your database fakes deletion, but if it's anything like personalized data, you are very likely violating the GDPR with your database. – cmaster - reinstate monica Dec 14 '18 at 17:09
  • 1
    Another thing to be aware of is that C# has *named parameters*, so you can also do `searchEntry("somesearch", searchDelete: true);` (handy if you have to deal with a 3rd party API that makes use of optional parameters the purpose of which may be unclear in a call). – Filip Milovanović Dec 14 '18 at 20:20

2 Answers2

17

My advice is to avoid optional parameters, especially when they are just switches. Instead, create two functions that clearly describe what they do:

void searchEntry(string searchValue)
void searchEntryIncludeDeleted(string searchValue)

That way, you avoid a boolean parameter, avoid the need for an enum, avoid optional parameters and make the code easier to read as a bonus.

To avoid code duplication, both methods then just call a private method that does the work. In the case of the private method, that boolean parameter becomes acceptable as the "distance" between the declaration and the caller is very small, thus avoiding the issues with it being non obvious what the boolean represents, eg:

public void searchEntry(string searchValue)
{
    searchEntries(searchValue, false);
}

public void searchEntryIncludedDeleted(string searchValue)
{
    searchEntries(searchValue, true);
}

private void searchEntries(string searchValue, bool includeDeleted)
{
    ...
}

And because it's private and only used by the two public wrapper methods, there is no need for an optional parameter.

David Arno
  • 38,972
  • 9
  • 88
  • 121
  • 2
    +1, speaking method names are the way to go as long as you have *few* different options. If you have *many* orthogonal ways of calling a method, it's time to create a parameter class or even a method object. – Kilian Foth Dec 14 '18 at 15:25
  • 2
    @KilianFoth, good point. `searchEntryIncludedDeletedInLastThreeDaysMatchingSuppliedPatternIgnoringCase` might be taking things too far :) – David Arno Dec 14 '18 at 15:27
  • Thanks, that looks good. If this is not a new question: I want to avoid code doublication, since exclusion of "deleted" is additional query to get all elements. Would it make sense that "SearchEntry" calls "SearchEntryIncludeDeleted" and remove the deleted in its part only? – Christian Müller Dec 14 '18 at 15:31
  • @ChristianMüller Oh yes, absolutely! Otherwise you'd be repeating an awful lot of code - which would be much worse than an occasional boolean parameter. – Kilian Foth Dec 14 '18 at 15:34
  • 1
    This can become ugly fast imo, if you have several switches. In that case an options object is sometimes used or optional parameters if the language supports them – Alvaro Dec 14 '18 at 17:48
  • @Alvaro, even if the language supports optional parameters, I'd still personally avoid them and would opt for an options object when multiple switches are needed. Just personal opinion though. – David Arno Dec 17 '18 at 09:37
  • @DavidArno definitely, but dynamically typed languages won't support that, so for example the default way to do this in python is using optional parameters – Alvaro Dec 18 '18 at 18:08
  • This only works if you are allowed to specify only one optional parameter per call. Otherwise you have to starting every combination of possible optional values. – Graham Dec 19 '18 at 17:45
7

The usual advice is to avoid many value type parameters and bool in particular as you can end up with arguably hard to read code

var x = searchEntry(
    true,
    false,
    "helloworld"
    )

var y = searchEntry(
    false, //super important that this is different!!
    false,
    "goodbye"
    )

But c# now has named arguments (sometimes incorrectly refered to as named parameters)

var y = searchEntry(
    includeDeleted: false,
    recursive: false,
    searchTerm: "goodbye"
    )

Which addresses the issue without having to create extra class/enums

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • Thank you ewan. That's what also I thinking about :) But I was in discussion, that it would be not good "clean-code" practice to use it like this and change the code. So it would be nice, if you can give some arguments, why this maybe clean enough (as I think). – Christian Müller Dec 14 '18 at 15:34
  • 2
    hmm, well 'clean code' really comes down to what _you_ find easy to understand. I would argue that in the named parameters case, we have nice long plain english names which make sense in combination with the method name and avoid the problem of many parameters making for an overly long method name. I dont think anyone would argue that it wasnt 'clean' the argument is whether a long method name is 'cleaner' – Ewan Dec 14 '18 at 15:43
  • Thank you ewan also for your answer. I like it, too. I go with David's solution, since it gives another direction I was thinking :) I wish I could approve both, so I had to decide.... – Christian Müller Dec 14 '18 at 15:55
  • omg this is just like "UKs Got Talent" all over again! – Ewan Dec 14 '18 at 16:02
  • Yes, but all could be glad, that I don't want to try singing :D – Christian Müller Dec 14 '18 at 16:07
  • A dev team talent competition is as good a way as any to resolve naming arguments :) – Ewan Dec 14 '18 at 16:08
  • Nitpick: They're named *arguments*. *Parameters* are in the method *declaration*, not *call*. – Furkan Kambay Dec 14 '18 at 17:32
  • I believe that Clean Code was written with Java in mind, which does not or did not have named arguments at the time (I'm not a Java guy). I wish there was a C# keyword to enforce that named arguments be used at the calling location! – Graham Dec 14 '18 at 17:40
  • @Graham could make sense. also for other types. maybe open an issue on github? – Furkan Kambay Dec 14 '18 at 17:46
  • I think the mistake of fluent interfaces is in trying to enforce how others use your code. – Ewan Dec 14 '18 at 17:49