1

I need some help. It is pretty easy. I have this piece of code, and I would like to discuss if it is correct, or if you suggest a better way to do it. I have an idea about the answer but I would like to see your answers. here it goes

if (myObject is ClassA)
{
    var myObjectA = myObject as ClassA;
    myObjectA?.MethodJustInA();
}
else if (myObject is ClassB)
{
    var myObjectB = myObject as ClassB;
    myObjectB?.MethodJustInB();
    myObjectB?.OtherMethodJustInB();
}

I think there is no need to create a new object after every if, just doing:

(ClassB)myObjectB.MethodJustInB();

And there is no need to check it is null since if it is within the if is because is not null

Thanks

Florian Schaal
  • 2,586
  • 3
  • 39
  • 59
Trexer
  • 21
  • 2
  • 5
    You don't need `as` if you already have checked the type, then you can cast it directly. The `as` operator comes in handy if the cast could fail which is impossible here. – Tim Schmelter Feb 09 '17 at 12:16
  • 1
    *no need to create a new object*, your not, your casting an existing object. They are reference types and therefore mutable – Liam Feb 09 '17 at 12:17
  • @TimSchmelter Is it bad to not do so? Like, I can either do it like this: `(ClassA)myObject.MethodJustInB();` or `(myObject as ClassB).MethodJustInB();`. Whats the difference in that case? – Cataklysim Feb 09 '17 at 12:18
  • 1
    You are not creating a new object by using as, and to call MethodJustInB you have to do it like this: ((ClassB)myObjectB).MethodJustInB(); – Tibi Feb 09 '17 at 12:18
  • 1
    `as` doesn´t create a new *object* but simply *references* an allready existing one. – MakePeaceGreatAgain Feb 09 '17 at 12:18
  • StackOverflow isn´t the right place to *discus* good or bad practices. – MakePeaceGreatAgain Feb 09 '17 at 12:21
  • 2
    @Cataklysim: what is _bad_? It is a little bit confusing because you suggest that you don't know if the type is really `ClassA` or `ClassB`. That's not true because you have checked it before. So someone else reading your code might try to safeguard this against `null`. Either use `as` with `null`-check or use `is` with explicit cast. – Tim Schmelter Feb 09 '17 at 12:22

5 Answers5

4

There are some optimizations possible.

  • If myObject is ClassA, you don't need the soft cast. Instead you can do the cast directly: var myObjectA = (ClassA)myObject;.
  • Since that is the case, and you just call a single method, you don't need to assign a new variable: ((ClassA)myObject)?.MethodJustInA();.
  • And because myObject is ClassA evaluates to false if myObject is null, you don't need to do the check again: ((ClassA)myObject).MethodJustInA();.

So:

if (myObject is ClassA)
{
    ((ClassA)myObject).MethodJustInA();
}
else if (myObject is ClassB)
{
    var myObjectB = (ClassB)myObject;
    myObjectB.MethodJustInB();
    myObjectB.OtherMethodJustInB();
}
Patrick Hofman
  • 153,850
  • 22
  • 249
  • 325
  • But when using `is` you try to cast object to some type, after condition with `is` returns `true` you should cast second time. It is better to use `as` and then check for `null` - in this case you cast only time. – Roman Feb 09 '17 at 12:24
  • 1
    @Roma "when using is you try to cast object to some type" > not true. You test the type, a much simpler operation than casting and assigning. – Patrick Hofman Feb 09 '17 at 12:27
  • 1
    In http://stackoverflow.com/questions/686412/c-sharp-is-operator-performance/686431#686431 they say that `is` and `as` actually *do* the same things, at least in negative case, see comment by Konrad Rudolph. – MakePeaceGreatAgain Feb 09 '17 at 12:36
  • Not sure if that is authoritative, but okay @HimBromBeere – Patrick Hofman Feb 09 '17 at 12:38
  • me neither, I don´t know the IL which is created for an as-cast. For `is` it´s a call to `isinst` obviously. Apart from this I allways prefer to use the `as`cast and check for `null`. – MakePeaceGreatAgain Feb 09 '17 at 12:40
  • @PatrickHofman, you can see the IL code and you will see same operation are performed using `is` and `as`. Classes - http://prntscr.com/e6ia8a, IL generated - http://prntscr.com/e6ib6f – Roman Feb 09 '17 at 12:52
  • 1
    Indeed, the `isinst` does the `is` (so it seems `as` does `is` first), but `as` also does the assignment, what `is` doesn't. @Roma – Patrick Hofman Feb 09 '17 at 13:01
  • @PatrickHofman, yes, doing `if (myObject is ClassA)` you cast one time and then `((ClassA)myObject)` you cast second time. You can use `as` and check for `null` and it will be only one cast. But anyway, it is great answer. – Roman Feb 09 '17 at 13:06
  • 1
    Here we have a statement from Eric Lippert: https://blogs.msdn.microsoft.com/ericlippert/2010/09/16/is-is-as-or-is-as-is/ – MakePeaceGreatAgain Feb 09 '17 at 14:09
  • So is is as @HimBromBeere – Patrick Hofman Feb 09 '17 at 14:19
  • Obviously, because both perform `isinst` under the hood, but `as` also returns its value while `is` just ommits the casted value. – MakePeaceGreatAgain Feb 09 '17 at 14:21
3

If you're using it this way, you only need to cast it ones:

var myObjectA = (myObject as ClassA);

if (myObjectA != null)
{
    myObjectA.MethodJustInA();
} 
else
{
    var myObjectB = (myObject as ClassB);

    if (myObjectB != null)
    {
        myObjectB.MethodJustInB();
    }
}

And in C# 7.0 you can do:

if (myObject is ClassA myObjectA)
    myObjectA.MethodJustInA();
else 
    if (myObject is ClassB myObjectB)
        myObjectB.MethodJustInB();
Jeroen van Langen
  • 21,446
  • 3
  • 42
  • 57
2

I prefer to cast with as and then check for null, this way you can skip the is check. Also, you do not need the elvis operator ?. because you know the object is not null.

var myObjectA = myObject as ClassA;
var myObjectB = myObject as ClassB;
if (myObjectA != null)
{
    myObjectA.MethodJustInA();
}
else if (myObjectB != null)
{
    myObjectB.MethodJustInB();
    myObjectB.OtherMethodJustInB();
}
Georg Patscheider
  • 9,357
  • 1
  • 26
  • 36
  • 3
    My experience is that this can get quite messy if you have more than a few possible types to check on, but generally a good solution. – Patrick Hofman Feb 09 '17 at 12:23
  • Yes, with this pattern there might be some unnecessary casts and variables in the scope outside the if-else, for example if the first `if` evaluates to true, the cast to ClassB is not needed. – Georg Patscheider Feb 09 '17 at 12:26
  • Indeed, you can still move the second cast to the `else`, but then you might end up with an endless of nested `if..else` blocks. – Patrick Hofman Feb 09 '17 at 12:28
1

If you are testing for multiple types, then as+null check is more efficient (just one cast, instead of is + casting):

var a = myObject as ClassA;
if (a != null)
    a.MethodJustInA();
var b = myObject as ClassB;
if (b != null)
    b.MethodJustInB();

In given scenario I'd even make local scope like this

{
    var obj = myObject as ClassA;
    if (obj != null)
        obj.MethodJustInA();
}
{
    var obj = myObject as ClassB;
    if (obj != null)
        obj.MethodJustInB();
}

{ } make it possible to reuse same variable name (easier to copy/paste and, in my oppinion, read).

I was a bit rushy and haven't thought well about else case (when myObject is ClassA you don't want to cast it as b, etc.). Normally I'd do return after each successful if and corresponding method call. I am not able to construct nice looking if/else if code otherwise.

Another idea is to use C# 6.0 null-conditional operator:

(myObject as ClassA)?.MethodJustInA();
(myObject as ClassB)?.MethodJustInB();

That looks really neat, but it will do unnecessarily casting to B and has side-effect: if ClassB inherits ClassA, then both methods will be called because both casts will success.

Note: mentioned side effect unfortunately applies to all proposed snippets.

Sinatr
  • 20,892
  • 15
  • 90
  • 319
  • The local scoping might give problems when `ClassB` derives `ClassA`. – Patrick Hofman Feb 09 '17 at 12:32
  • @PatrickHofman, `OP` uses `else if` (in his case that would be a problem too), but you are right, also see edit. My answer is not the best, but I don't think your is *ideal* too ;) – Sinatr Feb 09 '17 at 12:35
  • @PatrickHofman, wait, do you mean what it will successfully calls **both** methods? Yep, I see it now. – Sinatr Feb 09 '17 at 12:42
0

Within those if statements you can just cast your object like this:

var myObjectB = (ClassB)myObject;

or cast it directly:

((ClassB)myObject).MethodJustInB();
Metoniem
  • 239
  • 2
  • 15