Skip to main content
Code Review

Return to Revisions

1 of 2
Peter Csala
  • 10.7k
  • 1
  • 16
  • 36

Disclaimer: I could not run it locally (because I'm working on a Mac) or via dotnet fiddle (which works on some Linux distro) because Gdip threw a PlatformNotSupportedException. So, my below suggestions might break when you run it, but I tried to do my best to apply transformations that did not change the original behavior.


Main

  • Wrapping the entire main logic into a try-catch block is quite common.
static void Main()
{
 try
 {
 MainCore();
 }
 catch (Exception ex)
 {
 Console.Clear();
 Console.WriteLine($"Error: {ex}");
 Console.ReadLine();
 }
}
  • But in this case the catch block does not add too much value IMHO compared to the default behavior
    • Default: If exception is thrown then the exception's ToSting is written to the standard error output stream and application terminates. Previous console messages are still there.
    • In your case the thrown exception's ToString is written to the standard output stream and the application's termination is "suspended" until a key stroke. Previous console messages are gone.
  • IMHO this try-catch adds unnecessarily logic to your application.

Downloading image

  • I would suggest to extract this logic into its own method
static Bitmap GetImage()
{
 Bitmap image;
 do
 {
 image = null;
 Console.Clear();
 Console.Write("Image Path/URL: ");
 string imagePath = Console.ReadLine().Trim();
 
 if (Path.Exists(imagePath)) 
 {
 image = new(imagePath);
 }
 else if (imagePath.StartsWith("http"))
 {
 Console.WriteLine("Preparing image...");
 using WebClient client = new();
 byte[] imageData = client.DownloadData(imagePath);
 using MemoryStream stream = new(imageData);
 image = new Bitmap(stream);
 }
 if (image == null)
 {
 Console.WriteLine("Invalid path/URL. Press Enter.");
 Console.ReadLine();
 }
 } while(image == null);
 return image;
}
  • I've also changed your logic from an infinite while loop to a simpler do-while loop without any break statement
  • I would suggest to use Uri.TryCreate instead of checking for http prefix
  • I would also highly suggest to prefer HttpClient over WebClient

Handling menu

  • Same as previously, extract the logic into its own method
static void InvokeChosenConverter(Bitmap image)
{
 bool rerun;
 do
 {
 rerun = false;
 Console.WriteLine("Scaling Mode:");
 Console.WriteLine("[1] Default");
 Console.WriteLine("[2] High Quality (Stretches Image)");
 Console.WriteLine("[3] Specific Character Count");
 Console.WriteLine("[4] Specific Character Count + High Quality (Stretches Image)");
 ConsoleKeyInfo keyInfo = Console.ReadKey();
 Console.Clear();
 switch (keyInfo.Key)
 {
 case ConsoleKey.D1:
 ConvertToASCII(image, 2);
 break;
 case ConsoleKey.D2:
 ConvertToASCII(image, 1);
 break;
 case ConsoleKey.D3:
 ConvertToCharacterCountASCII(image, 2);
 break;
 case ConsoleKey.D4:
 ConvertToCharacterCountASCII(image, 1);
 break;
 default:
 Console.WriteLine("Invalid Input. Press Enter.");
 Console.ReadLine();
 Console.Clear();
 rerun = true;
 break;
 }
 } while(rerun);
}
  • I would consider to use multi-line string to print the menu
 Console.WriteLine(@"Scaling Mode:
 [1] Default
 [2] High Quality (Stretches Image)
 [3] Specific Character Count
 [4] Specific Character Count + High Quality (Stretches Image)");
  • It is probably just matter of taste but for this simple use case I've found a if-else if-else structure more legible
if (keyInfo.Key == ConsoleKey.D1 || keyInfo.Key == ConsoleKey.D2)
{
 ConvertToASCII(image, keyInfo.Key == ConsoleKey.D1 ? 2 : 1)
}
else if (keyInfo.Key == ConsoleKey.D3 || keyInfo.Key == ConsoleKey.D4)
{
 ConvertToCharacterCountASCII(image, keyInfo.Key == ConsoleKey.D3 ? 2 : 1)
}
else
{
 Console.WriteLine("Invalid Input. Press Enter.");
 Console.ReadLine();
 Console.Clear();
 rerun = true;
}
  • Since you mentioned that you are semi-new to programming I will not suggest more complex structures to utilize here.

With the above suggestions your Main would like this

static void Main()
{
 while (true)
 {
 Bitmap image = GetImage();
 InvokeChosenConverter(image);
 Console.ReadLine();
 Console.Clear();
 }
}

ConvertToASCII

  • Since C# 9 you can rewrite else if statements to a more terse expression
string character = brightness switch
{
 > 80 => "█",
 > 70 => "▌",
 > 60 => "▀",
 > 50 => "#",
 > 40 => "&",
 > 30 => "o",
 > 20 => "+",
 > 10 => ";",
 _ => "." 
};
Console.Write(character);

ConvertToCharacterCountASCII

  • I would suggest to use double.TryParse rather than Convert.ToDouble with try-catch block
double characterCount;
bool validInput = false;
do
{
 Console.Clear();
 Console.Write("Character Count: ");
 validInput = double.TryParse(Console.ReadLine().Trim(), out characterCount);
 
 if(!validInput)
 {
 Console.Clear();
 Console.WriteLine("Invalid Number. Press Enter");
 Console.ReadLine();
 }
} while (!validInput);
Peter Csala
  • 10.7k
  • 1
  • 16
  • 36
default

AltStyle によって変換されたページ (->オリジナル) /