2

I have the following method that opens a Excel workbook and returns it to be used in another method.

private Excel.Workbook openWorkbook()
    {
        // Get excel file path returns the file path of the excel workbook if it exists, otherwise returns null.
        List<string> filePaths = getExcelFilePath();
        if (filePaths != null)
        {
            return excel.Workbooks.Open(filePaths[0]);
        }
        return null;
    }

As you can see, I'm returning null to avoid a try-catch for a non-exixtent workbook when I call this from another method. Is it bad practice to do this. I do a similar thing in the following method which is supposed to return a list:

private List<string> getSOsToDelete()
    {
        // rawData is private variable in the class. If the workbook was not open this worksheet is set to null in another method similar to openWorkbook() above.
        if (rawData != null)
        {
            List<string> ToDeleteSOs = new List<string>();
            for (int i = rawData.Cells.SpecialCells(Excel.XlCellType.xlCellTypeLastCell, Type.Missing).Row; i > 1; i--)
            {
                if (rawData.Cells[i, 7].Value2.ToString() != "B2B" || rawData.Cells[i, 7].Value2.ToString() != "" || rawData.Cells[i, 8].Value2.ToString() != "Trns-Inc" || rawData.Cells[i, 8].Value2.ToString() != "")
                {
                    string SONumber = rawData.Cells[i, 3].Value2.ToString();
                    ToDeleteSOs.Add(SONumber);
                }
            }
            return ToDeleteSOs;
        }
        return null;
    }

If not so, what is the best way to write methods like these? For part 2 I guess I could return an empty list and check for length. I'm not sure which is better. However, for first method, I'm really not sure what to return if the file doesn't exist.

kovac
  • 4,945
  • 9
  • 47
  • 90
  • 1
    There's nothing wrong with returning `null` when it makes sense (as it does in your first function), but the real question is why are you trying to avoid a `try..catch`? – txtechhelp Aug 02 '16 at 04:51
  • 1
    Answers to these are going to vary greatly because there isn't enough information here to provide an informed opinion. Null from OpenWorkbook() should be fine. Unless the workbook must exist, in which case throwing an exception might be the better answer. – Jeff Siver Aug 02 '16 at 04:51
  • Actually, I'm trying to just pass the `null` to a public method so I can handle the absence of workbook or a worksheet there. This way, I can just handle this particular exception one-time by prompting the user to select a valid file. This reduces clutter in the code and also have heard reducing `try-catch` is better performance-wise. – kovac Aug 02 '16 at 04:57
  • `try...catch` block are really only "expensive" when there's an actual exception (in just about any language). In a tight loop it might make sense to try and only have 1 block or so, but given the code you posted, your file I/O is more expensive than a `try..catch` block. – txtechhelp Aug 02 '16 at 05:01
  • Possible duplicate of http://stackoverflow.com/questions/175532/should-a-retrieval-method-return-null-or-throw-an-exception-when-it-cant-prod – Hamid Pourjam Aug 02 '16 at 05:17
  • My rule for this is: don't use exceptions to control the flow of the program... use them to inform of errors that are -exceptions- : if you are opening a workbook with a filepath supplied by a user, and it doesn't exist: that's not an exception, it's the normal flow of the program. If you are opening a workbook that should be installed with your program and **should always exist** unless something **exceptional** has happened (i.e., some user deleted it, a corrupt install, not enough memory, etc.), then it's an exception if you can't find it. – Jcl Aug 02 '16 at 15:18
  • Oh I see... Okay i get it. I actually went with returning null as in the first code example. This is a good rule to have in mind. Thanks. – kovac Aug 02 '16 at 16:41

1 Answers1

1

I think there is no hard and fast rule for this. But I would return null from first method as returning a null where there is no workbook seems meaning full as null represent no object but from second method empty list would be a batter option as with list we used to use Count before using it instead of null or iterate over list etc. This also goes with convention like ToList will return list of zero elements instead of returning null.

Return null to avoid unknown exceptions from a method in C#

There could be different way to tell the caller method that error occurred in called method and exception is one of those and widely adopted. You can document the method to tell when kind of exception are expected from the method. Lets look at documentation of String.Substring Method (Int32, Int32) on MSDN. The documentation mentions that this method could through ArgumentOutOfRangeException when startIndex plus length indicates a position not within this instance, or startIndex or length is less than zero, MSDN

Adil
  • 146,340
  • 25
  • 209
  • 204
  • Thanks for your very clear explanation Adil. I tried to modify my code according to your explanation. Do let me know if this is acceptable. Thanks. – kovac Aug 02 '16 at 06:16
  • It would be better to add modified code as a part of your question by editing your question instead of answer you have given below. If Open method returns null/exception if file could not be opened then its fine? otherwise your previous method definition is fine. You have to change the second method by returning List as per my suggestion. To return string List you can declare ToDeleteSOs outside if you access it outside. This way you will have only one return statement return ToDeleteSOs outside if. – Adil Aug 02 '16 at 06:35