C# console app that draws an image to the console using ASCII

Disclaimer: I could not either 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); 

I'm semi-new to programming and want to improve my skills by understanding how people might optimize their code and how the code should be structured

Excellent. First bit of advice: Always start with the latter. It is much easier to optimize a well-structured program than it is to restructure an aggressively optimized program.

The answer of Peter Csala is quite good and I won't repeat his good points except to expand upon them slightly:

• Any time you see a loop with a long body, and particularly a loop with another loop in it, ask yourself "what business operation does the loop perform? what business operation does the loop body perform?" (By "business" I mean some task related to whatever the "business" of the program is: downloading a file, updating a database, generating a chess move...)

Odds are pretty good that the business of the loop or the loop body or both can be extracted to its own method. Doing so will make the code easier to read, reason about, test, maintain and re-use.

• Once you have extracted code into smaller methods, try to separate the "mechanism" logic from the "user interface" logic. Peter Csala's suggestion to extract GetImage is a good first step but let's go farther:

---

static Bitmap PromptForImage() { while(true) { Console.Clear(); Console.Write("Image Path/URL: "); string imagePath = Console.ReadLine().Trim(); Bitmap image = GetImage(imagePath); if (image != null) return image; Console.WriteLine("Invalid path/URL. Press Enter."); Console.ReadLine(); } } static Bitmap GetImage(string path) { Bitmap image; if (Path.Exists(path)) return new Bitmap(path); if (path.StartsWith("http")) { using WebClient client = new(); byte[] data = client.DownloadData(path); using MemoryStream stream = new(data); return new Bitmap(stream); } return null; } 

---

Notice how the logic gets much shorter and easier to follow when you separate user interface logic from your mechanisms. You can now independently test and improve GetImage, and reuse it in other programs without having to worry about how it impacts your user interface code.

Let's now dig into this new version of GetImage a bit.

• Don't try to be smarter than the library authors.

You check to see if the path exists before passing it to new BitMap. Why? Or, better question, what can go wrong? The path could exist but be a directory. The path could exist but be a file the user can see exists but not read. The path could be a readable file with an exclusive lock by another process. The path could be a link to a missing file -- yes, Path.Exists will say the path exists if the link is there but the linked file is missing. The path could exist but not contain a bitmap. There are a million things that can go wrong here, so instead of checking for only one of them, check for none of them. Let the author of the Bitmap class be the one who validates the input.

Same thing for checking if the path starts with http. There are URIs that do not start with "http" but you're not allowing the user to use them. And there are invalid URIs that do start with "http", and you're allowing those through. Don't do the check at all. Let the implementer of WebClient.DownloadData do their work and catch the exception that results if the path is bad.

Write some new methods that each do one thing:

static Bitmap ImageFromPath(string path) { try { return new Bitmap(path) } catch(FileNotFoundException) { return null; } } static Bitmap ImageFromURI(string path) { try { using WebClient client = new(); byte[] data = client.DownloadData(path); using MemoryStream stream = new(data); return new Bitmap(stream); } catch (whatever exceptions downloaddata can throw) { return null; } } static Bitmap GetImage(string path) { Bitmap image = ImageFromPath(path); if (image != null) return image; return ImageFromURI(path); } 

Now once again you have methods that can be read, understood, tested, improved, maintained and reused very easily.

---

My point here is that once you start breaking your program down so that each method does one logical thing, separating mechanisms from user interfaces, the whole structure of the rest of the program also naturally improves. Loops get shorter and easier to understand, the program starts to use the jargon of its business domain, everything gets better.

I think that most of the important points have already been touched by the excellent existing answers; however, I think it's worth mentioning the possibility of abstracting part of the dependencies of the core code.

At its core, your code does one thing: it gets an image, and produces a string output from it.

At the same time, you need to interact with a lot of things that your main, business logic is not really interested in; you need to interact with the user, which you are now doing through the console, you need to read and handle the image format, which you are now doing through the Bitmap class, and you need to interact with the Web, via the WebClient. These are dependencies of your core code.

Now, your dependencies are tightly coupled with your core; one day, you could decide not to interact with the user through the console, for instance, but to offer your logic through a web API; or to support image formats other than the bitmap one; but, with the way you've structured the code, changing this stuff can be quite an involved process.

Now, abstracting away the user interaction part is definitely not easy; partly because of the way the code is structured, and partly because predicting what kind of user interactions you want to support can be quite tricky.

But, finding a way to abstract the Bitmap part, so that, for instance, you can easily support different image formats, that is much more immediate.

Instead of using directly a Bitmap, let's create an interface; as naming things is hard, I'll keep it simple:

public interface IImage { } 

Let's not worry right now of any implementation for it, let's just focus on finding a way to remove references to Bitmap.

Now, you want to change the type, where you've used, from Bitmap to that IImage:

string imagePath = ""; IImage image = null; //... static void ConvertToASCII(IImage image, int iterator) //... 

You have a couple of lines where you are creating the Bitmap itself; let's change those with a couple of methods:

static IImage BuildImageFromPath(string path) { throw new NotImplementedException(); } static IImage BuildImageFromStream(Stream stream) { throw new NotImplementedException(); } 

Just some placeholders, we'll worry about the implementation later. With those available:

if (Path.Exists(imagePath)) { image = BuildImageFromPath(imagePath); break; } else if (imagePath.StartsWith("http")) { Console.WriteLine("Preparing image..."); using WebClient client = new(); byte[] imageData = client.DownloadData(imagePath); using MemoryStream stream = new(imageData); image = BuildImageFromStream(stream); Console.Clear(); break; } 

Now, the main method should be error free; but, we have the two methods you were using; let's start with ConvertToASCII. First of all, the parameter won't be a Bitmap anymore, but an IImage. Then, it's a question of identifying what you need in this method from the image; you have 4 things:

1. You need its width
2. You need its height
3. You need the brightness at a certain coordinate
4. You want to be able to dispose the image

So, we'll encode those requirements in the interface:

public interface IImage : IDisposable { public int Width { get; } public int Height { get; } public float GetBrightness(int x, int y); } 

This interface requires just a small change to the core code, these two lines:

Color pixelColor = image.GetPixel(x, y); float brightnessUnrounded = pixelColor.GetBrightness(); 

become this one:

var brightnessUnrounded = image.GetBrightness(x, y); 

Almost done, last method, ConvertToCharacterCountASCII; again, the Bitmap parameter becomes an IImage. This time, we should only have one thing: the part where we create the new Bitmap, scaling it.

So, again, let's express this in the interface:

public interface IImage : IDisposable { public int Width { get; } public int Height { get; } public float GetBrightness(int x, int y); public IImage Scale(int newWidth, int newHeight); } 

And now, this:

ConvertToASCII(new Bitmap(image, newWidth, newHeight), iterator); 

becomes this:

ConvertToASCII(image.Scale(newWidth, newHeight), iterator); 

Alright, the main code now should compile. But, we have left behind the implementation of the interface, and the two not implemented methods.

When doing some refactoring of this kind, it's best to keep the changes to a minimum; so, in this first iteration, it's worth creating just one implementation that does exactly the same thing as before, handling just bitmaps. The easiest way to do this is through composition: in the constructor, we create a Bitmap instance, and all the methods/properties we implement are going to be redirected to that instance.

As we have two different ways to build the image, through a path or through a stream, we'll have multiple constructors.

public class BitmapImage : IImage { private readonly Bitmap _bitmap; public BitmapImage(string path) { _bitmap = new Bitmap(path); } public BitmapImage(Stream stream) { _bitmap = new Bitmap(stream); } private BitmapImage(Bitmap bitmap) { _bitmap = bitmap; } public int Width => _bitmap.Width; public int Height => _bitmap.Height; public void Dispose() { _bitmap.Dispose(); } public float GetBrightness(int x, int y) { var pixel = _bitmap.GetPixel(x, y); return pixel.GetBrightness(); } public IImage Scale(int newWidth, int newHeight) { return new BitmapImage(new Bitmap(_bitmap, newWidth, newHeight)); } } 

Now, it's just a question of updating the two methods left behind:

static IImage BuildImageFromPath(string path) { return new BitmapImage(path); } static IImage BuildImageFromStream(Stream stream) { return new BitmapImage(stream); } 

This means that, if you want to support other kind of formats, or if you want to swap out System.Drawing so your application can work on Linux, it is much easier to do so: you just need to update the BitmapImage class, or to create a new implementation.

Now, as a note, in a bigger application you'd probably avoid having the two methods we are using here to create the BitmapImage, and instead create something like an ImageFactory class that handles that; which has the additional advantage of letting you have a separate component that is able to choose which implementation you want to use, if you have more than one.

Something similar can be done to abstract away the remaining dependencies (the console, the webclient, even the file system).

ConvertToASCII

First, you're using Math.floor and storing the result as a double. I wouldn't be surprised if double comparison was more costly than integer comparison, so I'd suggest converting to integer.

Second, let me introduce you to lookup tables. You are computing some kind of percentage, out of which you derive 9 possible answers.

Since your intervals are very regular, what about creating an array of 10 chars, with all your possible outputs.

Then instead of making your value on a scale of 0 to 99, make it 0 to 9. And you can now print "lookupTable[value]" (where value is 0..9). Your lookupTable will have, based on your code, entry 8 and 9 matching the same character.

All of a sudden, you have removed up to 9 ifs.

Reference: "Branchless computing" from the book "The Art of Writing Efficient Programs" (partly quoting the subtitle: "An advanced programmer's guide [...]").