3

I have the following:

if (model.PartitionKey.Substring(2, 2) == "05" || 
    model.PartitionKey.Substring(2, 2) == "06")

I have more like this. Is there a more clean way to code this where I don't have to repeat model.PartitionKey twice ?

Joey
  • 344,408
  • 85
  • 689
  • 683
  • 1
    Anne, you should make clear the _reason_ you need it. Because of performance? Code readability? To make it easy to extend (for example with values to compare that come from configuration)? Answers shot to every direction but often they're not compatible. – Adriano Repetti Jul 06 '12 at 07:56

7 Answers7

11

What about this:

if (new string[]{"05", "06"}.Contains(model.PartitionKey.Substring(2, 2))
    // ...

That leaves you at liberty to keep the strings you are looking for in a nice list...

var lookingFor = new string[]{"05", "06"};
var substring = model.PartitionKey.Substring(2, 2);
if (lookingFor.Contains(substring))
{
    // ...
}

This will help a lot if the list of strings you are looking for gets longer than just two... Also, you can then add this to a set (HashSet<string>) for more efficient lookup - but test this first, as overhead can eat up gains.

Joey
  • 344,408
  • 85
  • 689
  • 683
Daren Thomas
  • 67,947
  • 40
  • 154
  • 200
9

For such kind of cases I use an Extension Method

public static bool In<T>(this T source, params T[] list)
{
   if (source = null)
       throw new NullReferenceException("Source is Null");

   return list.Contains(source);
}

and call it as

if (model.PartitionKey.Substring(2, 2).In("05", "06"))

Being an Extension Method we can call it for all types like

if(myintegervariable.In(3, 4));

OR

if(mybytevariable.In(23, 56, 34, 43, 87, 155));
Nikhil Agrawal
  • 47,018
  • 22
  • 121
  • 208
7
var keyString = model.PartitionKey.Substring(2, 2);
if (keyString == "05" || keyString == "06")
{
    // ...
}
Surfbutler
  • 1,529
  • 2
  • 17
  • 38
  • That's not optimization but obfuscation. – Tim Schmelter Jul 06 '12 at 07:51
  • Doesn't really solve the problem, if anything your introducing *more* code to the equation. – James Jul 06 '12 at 07:52
  • 3
    The OP merely wanted to avoid repeating model.PartitionKey twice. How is doing that once only then saving the result not optimization? – Surfbutler Jul 06 '12 at 07:54
  • 1
    Tim, James: It fits the question exactly (»won't have to repeat model.PartitionKey twice«) and extracting common subexpressions into a local variable is a viable optimisation. And I'd argue that it's not harder to read – what `temp` is, is pretty clear, especially so close together and the condition *is* easier to read. – Joey Jul 06 '12 at 07:54
  • 2
    @Joey: `temp` is never good naming. Consider that this variable might be used across a huge method. The point is, `modelPkYear` would be more fail-safe. – Tim Schmelter Jul 06 '12 at 07:58
  • @Tim I typed my solution in around 3 seconds - of course I wouldn't normally use 'temp' as a variable name, but in this situation, without knowing the real meaning of the original expression, and as a piece of demonstration code, I felt it fit the bill. Anyway, rather than hating, feel free to change it to something more meaningful if you like! – Surfbutler Jul 06 '12 at 08:02
  • 1
    @Surfbutler: But you've written your answer 15 minutes ago. So you had 15 minutes to improve it. – Tim Schmelter Jul 06 '12 at 08:06
  • @Tim: Changed 'temp' changed to 'subString'. – Surfbutler Jul 06 '12 at 08:31
  • @Surfbutler: I would have preferred `modelPkYear` as mentioned(since i assume that it's the two digit year) but anyway, undone downvote – Tim Schmelter Jul 06 '12 at 08:35
  • @Surfbutler still far too generic, `subString` doesn't make it any clearer as to what information variable is holding. Something like `keyString` is more clear. – James Jul 06 '12 at 08:36
  • 1
    Now changed variable name to 'keyString' :) – Surfbutler Jul 06 '12 at 08:39
5

I'm surprised nobody offered switch as a possible alternative :)

switch (model.PartitionKey.SubString(2,2)) {
  case "05":
  case "06":
    // do stuff
    break;
  // other cases
  default:
    // like an else
}

You can read more about it at MSDN

jessicah
  • 945
  • 10
  • 15
  • 1
    +1 for introducing something completely different. The problem is, that the question is not clear enough to know if this solution fits in. If there are more constants to be compared with, it may even be a very nice solution. – Stefan Steinegger Jul 06 '12 at 08:21
  • 1
    I don't feel replacing the existing code to a switch statement is going to make the OP's life any easier. If anything, this would probably become more of a hinderance having to write a switch statement all over the place. Unless of course it's tucked away into a re-usable method. – James Jul 06 '12 at 08:34
3

You can save the substring in a variable:

var substring = model.PartitionKey.Substring(2, 2);
if (substring == "05" || substring == "06")

Or you could use a regular expression:

if (Regex.IsMatch("^..0[56]", model.PartitionKey))

This probably depends a bit on how easily you can understand a regex while reading code.

Joey
  • 344,408
  • 85
  • 689
  • 683
  • 6
    -1 for a regex, IMO you should never use a regex for simple string manipulations like this. If anything using a regex will *complicate* the statement more. – James Jul 06 '12 at 07:57
  • 1
    I would use a simpler regex, eg `^..(?:05|06)`, if I went that route.. also care for `.` not matching everything in some languages. –  Jul 06 '12 at 08:00
  • James, that's quite subjective, I think. In this case I'd rather read a simple regex (which *is* trivial to understand) than a more complex statement. But as said, this depends a bit on your familiarity with regular expressions. – Joey Jul 06 '12 at 08:01
  • pst, introducing non-capturing groups adds more clutter than a character class, I guess. – Joey Jul 06 '12 at 08:02
  • `I'd rather read a simple regex` ... in general, maybe yes. Subjective, indeed. However, in _this_ particular case, calling the original expression a "complex" statement is quite a stretch. – Christian.K Jul 06 '12 at 08:20
  • @Joey/pst - I did say *IMO*, however, in general I still don't believe recommending a regex for something as trivial as this is a *simpler* alternative. Not only that, but could also hinder performance based on how intensive the regex is. – James Jul 06 '12 at 08:25
1

To aid readibility you could extract the Substring out into a variable and then test that:

var partitionKeyBits = model.PartitionKey.Substring(2, 2);

if (partitionKeyBits == "05" || partitionKeyBits == "06") {

}

But otherwise that is about it.

Michael Shimmins
  • 19,961
  • 7
  • 57
  • 90
0

if (new []{"05", "06"}.Contains(model.PartitionKey.Substring(2, 2))

the syntax might be far off, corrections are welcome :)

Edit: new []

Orkun
  • 6,998
  • 8
  • 56
  • 103