Proper usage of "using" statement in base64 encode method

Question

I used the example on here, but with little changes in order to write a base64 encoder method. I have a few points about the code:

1. I would like to use using statement, but I do not want to use inner "using" statements, so I use multiple variable declaration in a single "using" statement. Is that suitable in order to benefit from "using" statement for more than one object. Moreover, what are alternatives?

2. Inside the using statement block, I used different try-catch blocks for each operation that can be throw an exception. I did not catch specific exceptions first an just use Exception class, but my goal is just return true or false which indicates the success status of the method. Is it a good practice to use different try-catch blocks and use "return" statement inside these blocks for resource handling?

3. Also, I would like to use an extra try-catch block which surrounds the using blocks for the classes which have Dispose() methods that can throw exceptions also. This is also discussed here. Is that a common approach, or should I completely stop using using statements, for these cases.

 public static bool ConvertToBase64(string inputFile, string outputFile)
 {
 try
 {
 using (FileStream inputFileStream = new FileStream(inputFile, FileMode.Open), 
 outputFileStream = new FileStream(outputFile, FileMode.Create))
 {
 try
 {
 ToBase64Transform base64Transform = new ToBase64Transform();
 //Buffers for read/write operations
 Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
 Byte[] inputBuffer = new byte[inputFileStream.Length];
 //Offset to count the number of bytes transformed so far
 int inputOffset = 0;
 try
 {
 inputFileStream.Read(inputBuffer, 0, (int)inputFileStream.Length);
 }
 catch(Exception ex)
 {
 return false;
 }
 if (false == base64Transform.CanTransformMultipleBlocks)
 {
 while (inputBuffer.Length - inputOffset > base64Transform.InputBlockSize)
 {
 //Transform a block of input data
 base64Transform.TransformBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset, outputBuffer, 0);
 inputOffset += base64Transform.InputBlockSize;
 try
 {
 outputFileStream.Write(outputBuffer, 0, base64Transform.OutputBlockSize);
 }
 catch(Exception ex)
 {
 return false;
 }
 //Insert a new line after 76 characters
 if (inputOffset % 19 == 0)
 {
 byte[] newline = Encoding.ASCII.GetBytes(Environment.NewLine);
 outputFileStream.Write(newline, 0, newline.Length);
 }
 }
 }
 //Final block transform
 try
 {
 byte[] lastBlock = base64Transform.TransformFinalBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset);
 outputFileStream.Write(lastBlock, 0, base64Transform.OutputBlockSize);
 }
 catch(Exception ex)
 {
 return false;
 }
 if (false == base64Transform.CanReuseTransform)
 {
 base64Transform.Clear();
 }
 }
 catch(Exception ex)
 {
 return false;
 }
 }
 }
 catch(Exception ex)
 { 
 return false;
 }
 return true;
 }

• Ant P: Your inner try/catch constructs are pointless since the outer one will already do exactly the same thing.
• Deniz: Yes, if you do do not any intention on logging specific errors or something else, inner try-catch blocks do not make much sense. Outer try block is mostly for using block when it calls Dispose method with a possible exception (not to override any exception occurred inside the using block).
• Malachi: @AntP just saw this comment after I posted my try/catch answer...lol

Answer 1

SRP

Your method is responsible of to many things, like

• reading from a file
• writing to a file
• converting to base64

So I would suggest to add some more methods

public static bool ConvertToBase64(string inputFile, string outputFile)
{
 try
 {
 byte[] input = System.IO.File.ReadAllBytes(inputFile);
 byte[] base64 = ConvertToBase64(input);
 System.IO.File.WriteAllBytes(outputFile,base64);
 return true;
 }
 catch(Exception)
 {
 return false;
 }
}
private byte[] ConvertToBase64(byte[] input)
{
}

As already stated in the comment, the inner try..catch statements are useless. Your outer try..catch would handle the exceptions in the same way.

About the using statements

If you don't need them, don't use them. As you are reading/writing to/from a file, you can just use the above methods, which internally using a using statement. Always use the right tool to do the job.

If you need to use using statements, you can also stack them like

using (disposable1)
using (disposible2)
{
}

Comments

Don't use comments to describe what you are doing, but why you are doing something. Especially if it is crystal clear what you are doing like

//Buffers for read/write operations
Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
Byte[] inputBuffer = new byte[inputFileStream.Length];

this comment is just useless.

Magic numbers

As you need a comment to describe

if (inputOffset % 19 == 0) 

you should better use a const for that 19 (but don't ask me how to name it)

If you still want the inner try..catch you can simply use catch(Exception) as there is no need for ex.

Otherwise your code looks good. The naming of your fields is also meaningful.

• Deniz: I just focused on using statement and exceptions, so I totally forgot about Single Responsibility Principle and Constants, but thanks especially SRP is a very good point. In addition to that, what if I do something special for different exception types and errors, there is still need to preserve inner try-catch blocks I think?
• Heslacher: What would you want to do for different exceptions ?
• Deniz: For example, I can use a different error codes for each exceptions while logging for better diagnosis.
• Heslacher: If you log the exception with the stacktrace you already know where which error occured. Also in the current state (using File.ReadAll..) you will clearly see which methods errors
• Piotr Kula: Also, dont block stack exceptions by writting new ones. let them all bubble up always (instead of blocking it like return false - this becomes critical ones you have interfaces so you can find the problem quickly) and always handle them on the top layer by implementing a logger, custom, nlog or whatever. So that if a user reports an error you have an easy way to look it up, with the original exception plus your new ones. Always include as much data as possible. Provide the user with an ID even so support can find it (or you)

Answer 2

You need to get rid of these try/catch statements, they are everywhere.

Use one Try Catch Statement

Instead of what you have

public static bool ConvertToBase64(string inputFile, string outputFile)
{
 try
 {
 using (FileStream inputFileStream = new FileStream(inputFile, FileMode.Open), 
 outputFileStream = new FileStream(outputFile, FileMode.Create))
 {
 try
 {
 ToBase64Transform base64Transform = new ToBase64Transform();
 //Buffers for read/write operations
 Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
 Byte[] inputBuffer = new byte[inputFileStream.Length];
 //Offset to count the number of bytes transformed so far
 int inputOffset = 0;
 try
 {
 inputFileStream.Read(inputBuffer, 0, (int)inputFileStream.Length);
 }
 catch(Exception ex)
 {
 return false;
 }
 if (false == base64Transform.CanTransformMultipleBlocks)
 {
 while (inputBuffer.Length - inputOffset > base64Transform.InputBlockSize)
 {
 //Transform a block of input data
 base64Transform.TransformBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset, outputBuffer, 0);
 inputOffset += base64Transform.InputBlockSize;
 try
 {
 outputFileStream.Write(outputBuffer, 0, base64Transform.OutputBlockSize);
 }
 catch(Exception ex)
 {
 return false;
 }
 //Insert a new line after 76 characters
 if (inputOffset % 19 == 0)
 {
 byte[] newline = Encoding.ASCII.GetBytes(Environment.NewLine);
 outputFileStream.Write(newline, 0, newline.Length);
 }
 }
 }
 //Final block transform
 try
 {
 byte[] lastBlock = base64Transform.TransformFinalBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset);
 outputFileStream.Write(lastBlock, 0, base64Transform.OutputBlockSize);
 }
 catch(Exception ex)
 {
 return false;
 }
 if (false == base64Transform.CanReuseTransform)
 {
 base64Transform.Clear();
 }
 }
 catch(Exception ex)
 {
 return false;
 }
 }
 }
 catch(Exception ex)
 { 
 return false;
 }
 return true;
}

Only Catch when you need to catch. Like this

public static bool ConvertToBase64(string inputFile, string outputFile)
{
 try
 {
 using (FileStream inputFileStream = new FileStream(inputFile, FileMode.Open), 
 outputFileStream = new FileStream(outputFile, FileMode.Create))
 {
 ToBase64Transform base64Transform = new ToBase64Transform();
 //Buffers for read/write operations
 Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
 Byte[] inputBuffer = new byte[inputFileStream.Length];
 //Offset to count the number of bytes transformed so far
 int inputOffset = 0;
 inputFileStream.Read(inputBuffer, 0, (int)inputFileStream.Length);
 if (false == base64Transform.CanTransformMultipleBlocks)
 {
 while (inputBuffer.Length - inputOffset > base64Transform.InputBlockSize)
 {
 //Transform a block of input data
 base64Transform.TransformBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset, outputBuffer, 0);
 inputOffset += base64Transform.InputBlockSize;
 outputFileStream.Write(outputBuffer, 0, base64Transform.OutputBlockSize);
 //Insert a new line after 76 characters
 if (inputOffset % 19 == 0)
 {
 byte[] newline = Encoding.ASCII.GetBytes(Environment.NewLine);
 outputFileStream.Write(newline, 0, newline.Length);
 }
 }
 }
 //Final block transform
 byte[] lastBlock = base64Transform.TransformFinalBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset);
 outputFileStream.Write(lastBlock, 0, base64Transform.OutputBlockSize);
 if (false == base64Transform.CanReuseTransform)
 {
 base64Transform.Clear();
 }
 }
 }
 catch(Exception ex)
 { 
 return false;
 }
 return true;
}

If there is an exception let it bubble up to where you want to catch it, not where it happens, you aren't even keeping the stack trace, so it doesn't matter where you catch it. so just do one try/catch statement here.

---

Don't use try/catch statements around using statements, there really is no need to do that, except in an instance like this where you want an exception to alter the output of the function like this. the only issue with doing this, is that the caller doesn't know why the conversion failed because the exception was caught and "handled".

As far as using a using statement around IDisposable objects, put all of them in using blocks, so if there is an exception or the application finishes correctly the object is disposed of properly and there is no memory leak. you don't even have to close the IDisposable object if you don't want to.

• Piotr Kula: Yea defienty one try is better but to further increase the readability its also good to test failures instead of success and wrapping in more brackets. So if something failes, throw exception, return, etc. Then just carry on with code within the same vertical line. I read this either from Scott Gu or some other clever guy, and it makes a massive difference in understanding the code later even if its yours.
• Malachi: @ppumkin, I am not sure that I am following what you are saying. the code is a Method that returns a true or false value, and each catch statement only returns false, so the point is that one catch in the method is sufficient to return that false value if anything goes wrong in the entire method. there is nothing that happens in the original code after an exception is caught.
• Piotr Kula: If you ever deal with interfaces, i mean like complex ones, say in a Facade architecture. You or your team mate will have an extremely difficult time trying to find out why it returned false. You blocked the exception, you cant tell the user a brief error, ir, file does not exist or if its something worse like, stackoverflow, how on earth are you going to know where to start looking. Blocking exceptions is considered bad practise. This should be void instead. and catch exceptions at a high level, log them, then trace them. The worst error message is "Undefined Error"
• Malachi: @ppumkin, I understand that, I mentioned it in my answer actually "the only issue with doing this, is that the caller doesn't know why the conversion failed because the exception was caught and "handled"."