Open hyblocker opened 2 years ago
Thanks for your feedback! Some stuff here is really useful. This project was mostly me messing around to learn C# so I welcome the feedback. Its probably going to take me a while to get through everything here but I'll be working on it.
Hi I'm literally only going to critique the installer a lot because I've been working on an installer for Amethyst for the past few months and seeing what this installer is like is just AAAAA. Anyway here's the big list of things that you should fix:
Why is the installer using .NET 6.0, and not .NET Framework? It's 150 MB, that's too large for an installer. Every Windows install since Windows 7 has .NET Framework pre-installed, take advantage of that (that means you don't need to include the entire runtime with the installer binary)! An installer should be tiny in file size and have 0 dependencies (it's an installer, the whole point is to setup everything on your system properly for whatever you're installing).
What's the design language you're going for? The installer has "fancy"-ish UI (more-on that in a bit) but is inconsistent in terms of design with the actual application. People like me who have OCD exist and this sort of inconsistency makes my blood boil.
What with the whitespace in the code? Clean it up!
Just wait for the file to download; This is bad practice.
Wrap the WebClient in a using statement so that it disposes when it goes out of scope. Then only use one WebClient for both of them.
Why are you using WebClient then HttpClient? HttpClient can make a GET request perfectly fine (see HttpClient.GetStringAsync() )
You can abstract opening a link to a method, you've got it 3 times already.
You should make an uninstaller too. Here's the documentation for the uninstaller registry keys from MSDN for reference. ALSO EXTREMELY IMPORTANT: make sure you don't format user's drives. PLEASE IMPLEMENT THIS IF YOU WRITE AN UNINSTALLER.
Also use IlMerge to merge .NET libraries together into a single binary. That way you can distribute a single exe file to your users, instead of a massive ZIP archive.
Use Checkbox.IsChecked instead of this. It's a literal property in the Checkbox class.
Nitpick: Use
Path.Combine("some/path", "directory");
andPath.GetFullPath("some\path/with\random/backslashes");
Sanitise the
folderPath
variable. I could type inAmong us
into the folder path and your installer will explode. TLDR: Make sure that the folder the user typed exists.The padding on the "Downloading..." button bugs me...
Consider making a successful installation more obvious, either by showing the user a dialogue box (see
MessageBox.Show()
) or something else that grabs the user's attention.