You have been posting in CodeReview for about 1 year, so while you are still learning C# and a hobbyist, you are not a total newbie. Therefore, you should expect the reviews to be a little more critical. Do understand that "critical" does not mean "mean spirited" but rather more blunt while also being constructive.
What disappoints me most in your code is its structure. You put almost everything in Main. I would challenge you to step back and look at the big picture of what goes on:
- UI Input - Ask for input values
- Business Logic - manipulate or
process those input values
- UI Output
So you could make your Main to look similar to those items. This is a major principle to keep in mind: Separation of Concerns, namely your business logic and UI should be separate. But Separation of Concerns also applies to breaking up a monster method that is doing too much into smaller methods where each call does a specific thing.
There is also the .NET guideline that you should limit the scope of a variable. I am an old-timer who worked with VB and way back then it was common to define ALL the variables at the top of a method and then use them later. With .NET, this is not needed. In particular, I am referring to the variable textWriter
. You define at the very top but start to use it at the bottom.
I also do not like the name textWriter
. I personally would use a generic name of writer
just in case you every change to a different implementation. But that really doesn't matter because you don't need that variable at all. You use 4 lines of code when only 1 is needed:
File.WriteAllLines(@System.Environment.GetEnvironmentVariable("OUTPUT_PATH"),
BigSorting(unsorted));
I have also observed in a history of your posts, and the same is true here, that you code to a specific implementation of array. Your methods can have a broader reach and re-usability if you coded to a more basic implementation, such as IList or even IEnumerable. Consider:
static IOrderedEnumerable<string> BigSorting(IEnumerable<string> items) => items.OrderBy(s => s.Length).ThenBy(s => s);
The input is no longer restricted to being an array, and I do not create an entirely new array for output. I also changed the parameter name to items
because I don't care if someone inputs a unsorted collection or one sorted in some other order. Doesn't matter to the method.
What if you wanted to output to an array? Then use .ToArray()
when you make the call BigSorting
rather than doing it inside BigSorting
.
So that addresses quick and simple sorting use LINQ, which produces a 2nd array. There is an alternative that doesn't require a 2nd array. I haven't tested with huge number of elements or crazy long values. I leave that to you and hopefully you will find it more performant.
I am referring to making a custom IComparer. See this link. Example:
public class NumericStringComparer : IComparer<string>
{
// https://docs.microsoft.com/en-us/dotnet/api/system.array.sort?view=net-5.0#System_Array_Sort__1___0___System_Collections_Generic_IComparer___0__
public int Compare(string x, string y)
{
// Compare x and y as numbers without leading zeroes.
if (x == null)
{
return (y == null) ? 0 : -1;
}
if (y == null)
{
return 1;
}
if (x.Length < y.Length)
{
return -1;
}
if (y.Length < x.Length)
{
return 1;
}
return x.CompareTo(y);
}
}
Now you would use it like:
Array.Sort(unsorted, new NumericStringComparer());
File.WriteAllLines(@System.Environment.GetEnvironmentVariable("OUTPUT_PATH"),
unsorted);
Except I now have a problem with the name "unsorted". While the array containing inputs was initially unsorted, I later sorted in-place to that very same array, so in WriteAllLines
the name choice of unsorted
is misleading. Thus, I would choose a generic name of items
for the array.