As the last week of Hacktoberfest comes to a close, I'm thrilled to share my recent contributions to the open-source community, particularly to the CrispyWaffle project, a toolkit for .NET projects. In this blog, I'll recount my journey and the changes I made to improve the project.
Getting Started
I kicked off my Hacktoberfest contributions by diving into CrispyWaffle, a project that caught my attention due to its importance in the .NET ecosystem. The maintainers had an open issue #211, which required assistance in removing a retry check within one of their classes. After discussing the changes and receiving their initial approval, they encouraged me to seek additional areas for improvement within the codebase. Eager to make a positive impact, I accepted the challenge.
The Code Refactoring
1. Removing the Retry Rule in FtpClient.cs
The first issue to tackle was in the FtpClient.cs file, This class used a while loop to manage retry attempts. Here's a comparison between the old and new code:
Old Code:
private bool CreateInternal(string path, byte[] bytes)
{
var result = false;
var attempts = 0;
while (true)
{
attempts++;
try
{
// ... (rest of the code)
}
catch (WebException e)
{
if (attempts >= 3)
{
throw new FtpClientException(path.GetPathOrFileName(), "create", e);
}
Thread.Sleep(1000);
}
}
return result;
}
New Code:
private bool CreateInternal(string path, byte[] bytes)
{
var result = false;
try
{
// ... (rest of the code)
}
catch (WebException e)
{
throw new FtpClientException(path.GetPathOrFileName(), "create", e);
}
return result;
}
Explanation: The old code used a while(true) loop for retry attempts, which is generally not a good practice. It also included a counter (attempts) to track the number of retries and a subsequent if statement to check if the maximum retry limit was reached. In the new code, I removed the loop and the unnecessary variables, simplifying the code and making it more readable.
2. Flipping NOT Conditions
In the original code, I noticed instances where conditions were negated, making them less intuitive. I proposed changing these NOT conditions to a more straightforward format:
Old Code:
request.Method = !string.IsNullOrWhiteSpace(uri.GetFileExtension())
? WebRequestMethods.Ftp.GetFileSize
: WebRequestMethods.Ftp.ListDirectory;
New Code:
request.Method = string.IsNullOrWhiteSpace(uri.GetFileExtension())
? WebRequestMethods.Ftp.ListDirectory
: WebRequestMethods.Ftp.GetFileSize;
Explanation: Writing code without negations is a better practice because it enhances code readability. The new code provides a more intuitive understanding of the logic, making it easier for other developers to follow.
3. Better Resource Handling in ExistsInternal
In the ExistsInternal method, I found several areas where resource handling could be improved:
Old Code:
private bool ExistsInternal(string path)
{
var result = false;
Stream responseStream = null;
try
{
LogConsumer.Info(
"Checking in FtpClient the path/file: {0}",
path.GetPathOrFileName()
);
var uri = new Uri(path);
var request = (FtpWebRequest)WebRequest.Create(uri);
request.Credentials = new NetworkCredential(_userName, _password);
request.Method = !string.IsNullOrWhiteSpace(uri.GetFileExtension())
? WebRequestMethods.Ftp.GetFileSize
: WebRequestMethods.Ftp.ListDirectory;
request.Timeout = 30000;
request.ReadWriteTimeout = 90000;
request.UsePassive = true;
var response = (FtpWebResponse)request.GetResponse();
var status = response.StatusCode;
responseStream = response.GetResponseStream();
if (responseStream == null)
{
throw new InvalidOperationException("Response stream is null");
}
using var reader = new StreamReader(responseStream);
responseStream = null;
while (!reader.EndOfStream)
{
_files.Enqueue(reader.ReadLine());
}
if (!string.IsNullOrWhiteSpace(uri.GetFileExtension())
&& status == FtpStatusCode.FileStatus
|| status == FtpStatusCode.OpeningData)
{
result = true;
}
}
catch (WebException)
{
result = false;
}
finally
{
responseStream?.Dispose();
}
return result;
}
New Code:
private bool ExistsInternal(string path)
{
try
{
LogConsumer.Info("Checking in FtpClient the path/file: {0}", path.GetPathOrFileName());
var uri = new Uri(path);
var request = (FtpWebRequest)WebRequest.Create(uri);
request.Credentials = new NetworkCredential(_userName, _password);
request.Method = string.IsNullOrWhiteSpace(uri.GetFileExtension())
? WebRequestMethods.Ftp.ListDirectory
: WebRequestMethods.Ftp.GetFileSize;
request.Timeout = 30000;
request.ReadWriteTimeout = 90000;
request.UsePassive = true;
using var response = (FtpWebResponse)request.GetResponse();
using (var responseStream = response.GetResponseStream())
{
using var reader = new StreamReader(responseStream);
while (!reader.EndOfStream)
{
_files.Enqueue(reader.ReadLine());
}
}
return string.IsNullOrWhiteSpace(uri.GetFileExtension())
? response.StatusCode == FtpStatusCode.OpeningData
: response.StatusCode == FtpStatusCode.FileStatus || response.StatusCode == FtpStatusCode.OpeningData;
}
catch (WebException)
{
return false;
}
}
Explanation: In the new code, I improved resource handling by using using statements to ensure that resources are properly disposed of when they are no longer needed. Additionally, I removed the result variable, simplifying the code and improving its clarity.
Wrapping Up
My Hacktoberfest journey has been both rewarding and educational. I've had the opportunity to contribute to the open-source community and make meaningful improvements to a valuable project. These changes not only enhance the project's functionality but also make it more accessible for other developers.
As a final note, I'm thrilled to share that my contributions have already been merged into the CrispyWaffle project. You can check out my Pull Request and the final merge commit. The CrispyWaffle team immediately approved and merged my proposed changes without any further change requests, underlining the collaborative spirit of the open-source community. As Hacktoberfest comes to an end, I look forward to more open-source contributions in the future.
Top comments (0)