best way to handle null objects

[mp]

this is a method in a class to provide access to Excel
where _xlApp etc are private member vars for the respective objects
they shouldn't ever be null by the time this is called...but just in case...

public Range GetRange(string rangename)
{
Range rng=null ;
if (_xlApp != null)
{if (_xlWorkBook != null)
{if(_xlWorkSheet!=null)
{
rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
}
}
}
return rng;
}

would it be advised to provide an else for each if
if (_xlApp != null)
{}
else
{throw exception or ???}
that would make the layered if clauses messy

or better to
if (_xlApp == null){...throw excep and bail}
if (_xlWorkBook == null){...throw excep and bail}
if (_xlWorkSheet == null){...throw excep and bail}
....aok so get the range...

thanks
mark

 

[Arne Vajhøj]

this is a method in a class to provide access to Excel
where _xlApp etc are private member vars for the respective objects
they shouldn't ever be null by the time this is called...but just in case...

public Range GetRange(string rangename)
{
Range rng=null ;
if (_xlApp != null)
{if (_xlWorkBook != null)
{if(_xlWorkSheet!=null)
{
rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
}
}
}
return rng;
}

would it be advised to provide an else for each if
if (_xlApp != null)
{}
else
{throw exception or ???}
that would make the layered if clauses messy

or better to
if (_xlApp == null){...throw excep and bail}
if (_xlWorkBook == null){...throw excep and bail}
if (_xlWorkSheet == null){...throw excep and bail}
...aok so get the range...

If _xlApp, _xlWorkBook and _xlWorkSheet are initialized in the
constructor then drop the test. If someone later screwup the
initialization, then that will result in a NullReferenceException.
But they could also mess up things 717 other ways that you
can not protect against, so there are really no point in
trying.

If they may not have been set then you should test. I
suggest to code it like:

public Range GetRange(string rangename)
{
if (_xlApp == null || _xlWorkBook == null || _xlWorkSheet == null)
{
throw new FubarException();
}
Range rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
return rng;
}

to keep the exception logic and the main logic better separated.

Arne

 

[Arne Vajhøj]

If _xlApp, _xlWorkBook and _xlWorkSheet are initialized in the
constructor then drop the test. If someone later screwup the
initialization, then that will result in a NullReferenceException.
But they could also mess up things 717 other ways that you
can not protect against, so there are really no point in
trying.

If they may not have been set then you should test. I
suggest to code it like:

public Range GetRange(string rangename)
{
if (_xlApp == null || _xlWorkBook == null || _xlWorkSheet == null)
{
throw new FubarException();
}
Range rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
return rng;
}

to keep the exception logic and the main logic better separated.

And I don't like the assignment to _xlRange!

I assume this is saved for some other method that will
be fubar if this method is not called first.

Arne

 

[mp]

[...]

thanks for the input

And I don't like the assignment to _xlRange!

I assume this is saved for some other method that will
be fubar if this method is not called first.

Arne

yes, i save it so later calls dont' have to pass in a range object
thinking this simplifys my calling code
but i also returned it in case client wanted to do other things
that weren't built in using the "default" range object
just beginning to design all this so i was leaving my options open
maybe not good
mark

 

[Arne Vajhøj]

yes, i save it so later calls dont' have to pass in a range object
thinking this simplifys my calling code
but i also returned it in case client wanted to do other things
that weren't built in using the "default" range object
just beginning to design all this so i was leaving my options open
maybe not good

Passing the argument is much better.

More readable, less dependency between method calls,
not thread unsafe etc..

Arne

 

[Registered User]

this is a method in a class to provide access to Excel
where _xlApp etc are private member vars for the respective objects
they shouldn't ever be null by the time this is called...but just in case...

public Range GetRange(string rangename)
{
Range rng=null ;
if (_xlApp != null)
{if (_xlWorkBook != null)
{if(_xlWorkSheet!=null)
{
rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
}
}
}
return rng;
}

would it be advised to provide an else for each if
if (_xlApp != null)
{}
else
{throw exception or ???}
that would make the layered if clauses messy

The code is already pretty messy

or better to
if (_xlApp == null){...throw excep and bail}
if (_xlWorkBook == null){...throw excep and bail}
if (_xlWorkSheet == null){...throw excep and bail}
...aok so get the range...

Are all those nested if statements really needed? It appears the only
value which needs to be checked for null is the _xlWorkSheet member.
I'm guessing if that member isn't null then neither are the _xlApp and
_xlWorkBook members.

public Range GetRange(string rangename)
{
Range rng=null ;
// validate argument as well
if(_xlWorkSheet!=null && !String.IsNullOrEmpty(rangename))
{
rng = (Range)_xlWorkSheet.get_Range(rangename,
System.Reflection.Missing.Value);
_xlRange = rng;
}
return rng;
}

Another consideration should be the need to raise an exception. I
would suggest evaluating the possibility of simply returning null. The
caller can check the return value before proceeding.

regards
A.G.

 

[mp]

this is a method in a class to provide access to Excel []
if (_xlApp != null)
{if (_xlWorkBook != null)
{if(_xlWorkSheet!=null)
{
rng = (Range)_xlWorkSheet.get_Range(rangename,

[]

Are all those nested if statements really needed? It appears the only
value which needs to be checked for null is the _xlWorkSheet member.
I'm guessing if that member isn't null then neither are the _xlApp and
_xlWorkBook members.

good point. at this point i was debug.printing a statement for each
condidtion
just for my own use as i work with coding this project, but in reality what
you say makes sense...

[]

Another consideration should be the need to raise an exception. I
would suggest evaluating the possibility of simply returning null. The
caller can check the return value before proceeding.

regards
A.G.

I originally was doing that, returning null, then (due to my inexperience)
I thought maybe that was tacky, as it made the caller have to do extra
checks???
But on further thought, the *only* way they could be null is if the caller
forgets
to "request" a workbook and worksheet before trying to get a range obect.
so i'm not really foisting extra work on the caller, just letting it know it
has
shirked its' duty somewhere upstream.

thanks for the input
mark

 

[Registered User]

this is a method in a class to provide access to Excel []
if (_xlApp != null)
{if (_xlWorkBook != null)
{if(_xlWorkSheet!=null)
{
rng = (Range)_xlWorkSheet.get_Range(rangename,

[]

Are all those nested if statements really needed? It appears the only
value which needs to be checked for null is the _xlWorkSheet member.
I'm guessing if that member isn't null then neither are the _xlApp and
_xlWorkBook members.

good point. at this point i was debug.printing a statement for each
condidtion
just for my own use as i work with coding this project, but in reality what
you say makes sense...

[]

Another consideration should be the need to raise an exception. I
would suggest evaluating the possibility of simply returning null. The
caller can check the return value before proceeding.

regards
A.G.

I originally was doing that, returning null, then (due to my inexperience)
I thought maybe that was tacky, as it made the caller have to do extra
checks???

There are many situations where null can be a valid return value. It
is seldom a bad idea for the caller validate a return value if the
return type is nullable.

One thing to be aware is an exception will be thrown if get_Range is
passed a bad argument e.g.
Range aRange = ws.get_Range("AG", "C7");
The happens because COM interop translates all error HRESULTS in to
COMExceptions. This can be avoided by using a regular expression to
validate the argument(s).

But on further thought, the *only* way they could be null is if the caller
forgets
to "request" a workbook and worksheet before trying to get a range obect.
so i'm not really foisting extra work on the caller, just letting it know it
has
shirked its' duty somewhere upstream.

I think this Excel wrapper type is a good learning exercise. I let you
discover the lessons as you go

thanks for the input

You're most certainly welcome

regards
A.G.

 

[mp]

this is a method in a class to provide access to Excel []

One thing to be aware is an exception will be thrown if get_Range is
passed a bad argument e.g.
Range aRange = ws.get_Range("AG", "C7");
The happens because COM interop translates all error HRESULTS in to
COMExceptions. This can be avoided by using a regular expression to
validate the argument(s).

oh no! <g> not regex again!
:-(
I'm so dumb when it comes to regex
(oh well, not *Just* regex <g>)

so i have to validate everything between "A1" up to "IV65536"
and everything in between
argghh

thanks for the suggestion though...i have way too much hair still left...
haven't pulled it *all* out yet
mark


[]

 

[mp]

[...]
so i have to validate everything between "A1" up to "IV65536"
and everything in between

No, you don't have to do that. Let Excel do the validation. Yes, you'll
get an exception. But that's by design.

If the user is providing the range address, then be prepared to catch the
exception and report it to the user. If the range address always coming
from your own program, then just make sure you don't generate invalid
addresses. If you do, you'll get an exception and your program will
crash. Again, this is a _good_ thing. You don't want your program to
keep running if it's not behaving correctly. You need to fix it so it
does.

Pete

much easier!
Thanks
mark

 

[Registered User]

Refactor your single class into multiple classes, each representing
(and maintaining a reference to) the individual Excel object that is
required for that particular class's responsibilities (which may turn
out to be, for some classes, only to retrieve the next object in the
sequence of related types, in which case you might rethink whether that
class is even really needed…just make the caller retrieve the Excel
interop type directly itself).

One of the lessons I was alluding to is determining the value of this
wrapper type. The layer of indirection can introduce more complexity
than it conceals. At some point it may revealed to be more expedient
to use all the COM-based Excel objects directly.

regards
A.G.

 

[mp]

Refactor your single class into multiple classes, each representing

[]
One of the lessons I was alluding to is determining the value of this
wrapper type. The layer of indirection can introduce more complexity
than it conceals. At some point it may revealed to be more expedient
to use all the COM-based Excel objects directly.

It may be, yes. I start with the assumption that Mark has a reason for
writing the "reader" code in the first place. Obviously, that code will
have to exist somewhere independently of the Excel objects.

yes, i have a reason, though maybe not a good one <g>
when I want to access an excel workbook in some current or future code
to me it's easier to remember how to write:
//-----------------------
ExcelReader xlrdr = new ExcelReader(workBookName, sheetName);xlrdr.Close();
xlrdr=null;
//-----------------------

as opposed to writing in every client that may need a workbook/sheet

//-----------------------
xlApp =
(Excel.Application)System.Runtime.InteropServices.Marshal.GetActiveObject("Excel");
try
{
_xlWorkBook = xlApp.Workbooks.Open(_xlWorkBookName, 0,
false, 5, "", "", true, Excel.XlPlatform.xlWindows, "\t", false, false, 0,
true, 1, 0);
}
catch
{
_xlWorkBook =
_xlApp.Workbooks.Add(System.Type.Missing );
_xlWorkBook.SaveAs(_xlWorkBookName,System.Type.Missing,
System.Type.Missing,
System.Type.Missing,
false,
false,
XlSaveAsAccessMode.xlShared,
System.Type.Missing,
System.Type.Missing,
System.Type.Missing,
System.Type.Missing,
System.Type.Missing);
}

if (_xlWorkBook != null)
{
...check for existence of sheet and create if not
found...
try {
_xlWorkSheet =
(Worksheet)_xlWorkBook.Worksheets.get_Item(worksheetname);
}
catch {} etc}
else
...handle error
... then when done: ...
go through teardown process from range to sheet to book to xlapp to
close excel
//-----------------------

But as the other of the alternatives I proposed suggests, that doesn't
mean that the Excel types themselves need to be encapsulated within a
custom wrapper. Rather, they may just be passed to a class that simply
operates on an existing Excel object.

so do you mean something like
instead of
if(xlrdr.GetRange(rangename))//<<<return bool
{xlrdr.SomeOperationOnRange}
do this
Range rng = xlrdr.GetRange(rangename);//<<<return range
this.SomeOperationOnRange(rng);

???
that probably does make more sense


And it would not be surprising

for that class to be essentially stateless, allowing it to be a static
class (of course, if there are configuration settings to control how the
Excel object is processed, then a stateful, non-static class might be
warranted instead).



On the other hand, wrapping a data source is a well-established and useful
pattern. For example, the TextReader and TextWriter sub-classes, which do
things like wrapping a Stream, String, or StringBuilder to perform i/o.
Or even the MemoryStream object which can wrap an array for a similar
purpose (at a different level of abstraction, of course).

Pete

I don't remember the pattern name but seem to recal something about
creating an interface that has only a limited number of the "properties"
the underlying object may actually have, when you know you don't
need the complexity of the original object??? I don't think it's adapter,
maybe facade?

thanks for all the ideas
mark

 

[Arne Vajhøj]

yes, i have a reason, though maybe not a good one<g>
when I want to access an excel workbook in some current or future code
to me it's easier to remember how to write:
//-----------------------
ExcelReader xlrdr = new ExcelReader(workBookName, sheetName);

{...do something with worksheet}<<<<<<<<<<<

xlrdr.Close();
xlrdr=null;
//-----------------------

as opposed to writing in every client that may need a workbook/sheet
[…boilerplate workbook code snipped…]

I wasn't really questioning whether the code is a good idea or not. I
was simply pointing out that assuming you're going to have the code, it
will have to exist somewhere (which seems like an obvious statement to
me…I made it just to preface the rest of my reply ).

But, I note that in the code you posted, there's not really any clear
motivation to have a single class in which all of that code lives. You
could in fact just return the Excel workbook object from a method that
handles the workbook part. Then you could return the worksheet from a
method that handles the worksheet part. You may or may not feel a need
for a third method that wraps the call to GetActiveObject(). But such
convenience methods are quite common and reasonable.

Doing it that way, you could simply have a class like this (I've left
out the actual code…it would just be the same code you posted, placed
appropriately):

static class ExcelHelpers
{
public static Excel.Application GetApplication() { … }
public static Excel.Workbook GetWorkbook(Excel.Application app, string
name) { … }
public static Excel.Worksheet GetWorksheet(Excel.Workbook workbook,
string name) { … }
}

void SomeMethod()
{
Excel.Application app = ExcelHelpers.GetApplication();
Excel.Workbook book = ExcelHelpers.GetWorkbook(app, "Some Workbook");
Excel.Worksheet sheet = ExcelHelpers.GetWorksheet(book, "Some Worksheet");
}

That way, the client code cannot possibly call the helper code without
the proper initialization having been done.

Removing a potential source for errors *and* make the code
more readable is a clear win.

And those fields being set by one method and used by another
method in the original code really make it hard to follow
what is going on.

Arne

 

[mp]

[]

Removing a potential source for errors *and* make the code
more readable is a clear win.

And those fields being set by one method and used by another
method in the original code really make it hard to follow
what is going on.

Arne

thanks to everyone for your help
mark

 

[mp]

yes, i have a reason, though maybe not a good one<g>

[]


Doing it that way, you could simply have a class like this (I've left out
the actual code.it would just be the same code you posted, placed
appropriately):

static class ExcelHelpers []

sounds good.
are you saying to put that in the client .cs file, as opposed to a separate
xl.cs file?
or just showing a better arrangement in the xl.cs file?
my reason for having a separate xlReader.cs (or xlhelper) was so i can drop
it in other
projects that come along and want easy access to excel.
(without me having to remember the excel specific syntax each time, or copy
paste the excel code into other clients)

That way, the client code cannot possibly call the helper code without the
proper initialization having been done.

makes obvious sense, i was just being brain dead(trying to learn too many
things at once)

If you like, you can even make the above use extension method syntax, so

[]

i'd need an extension for my brain....i'm not familiar with extensions but
that looks interesting
i always wondered what those code statements were that had extra args that
weren't
separated by commas...(this Excel.Application app, string name)

The bottom line here is that you should avoid designing your code such
that it's as difficult as possible to use it incorrectly.

i always make things as difficult as possible!
(but i knew what you meant)<g>

[]

That said, it's not clear to me you really need a facade here. It's
reasonable to allow the client code access to all of the members of the
Excel objects. You just (it seems to me) want to provide helper code to
make particular kinds of operations simpler.
yep


Finally, note that C# 4.0 has support for optional arguments. I don't
know what version of C# you're using,

2008

but the optional argument feature

was added specifically to help make this kind of interaction with Office
easier to deal with. You may find that using C# 4.0, adding these kinds
of helper methods isn't really all that compelling, because the direct
calls are almost as simple as you could make them yourself (because one of
the main complications - all of the extra arguments you don't really care
about - just goes away).

Pete

i've been thinking of getting 4.0 (I think that's 2010 version, right?)
i do miss optional args but i've got enuf on my plate at the moment

thanks as always
mark

 

[mp]

are you saying to put that in the client .cs file, as opposed to a
separate
xl.cs file?
or just showing a better arrangement in the xl.cs file?

I'm not saying either. But since you ask, no.I don't see any reason to
put the code in a different file than where you're putting it now. If you
want this code reusable, eventually you're going to want it in its own DLL
that you can reference from other projects. But that DLL project will
likely wind up being the home for your xl.cs file.

[...]

If you like, you can even make the above use extension method syntax, so

[]

i'd need an extension for my brain....i'm not familiar with extensions
but
that looks interesting
i always wondered what those code statements were that had extra args
that
weren't
separated by commas...(this Excel.Application app, string name)

No extra arguments. The "this" keyword in that context simply modifies
the first argument, identifying the entire method as an extension method.

[...]
i've been thinking of getting 4.0 (I think that's 2010 version, right?)
i do miss optional args but i've got enuf on my plate at the moment

Yes, C# 4.0 comes with VS2010.

Pete

thanks for all this
I'm learning tons.
now if i can put it to good use!
thanks
mark