[Logik]

Hi guys, I just finished coding a tool in vb.net that extracts icons from executables and assembly information from .NET executables.

I'm open to any advice or tips you may have on my little program and I hope you guys find it useful!

You can find the source code over on my github page

Some images of how it looks:
Spoiler (Click to View)


Spoiler (Click to View)


Spoiler (Click to View)

[MavenGradle]

There are certainly some comments that can be made about the state of your Git repository. First of all, why are these folders and data in said folder a thing?

https://github.com/LogikError/Icon-and-A.../obj/Debug
https://github.com/LogikError/Icon-and-A.../bin/Debug
https://github.com/LogikError/Icon-and-A...conLib/obj

These do not belong in a Git repository or on GitHub in general.

Second of all, the name Form1.vb doesn't really mean a whole lot. You should take proper care of your variable- and file names and actually give them meaningful names. That's not the only place where you don't use proper names;

Code

Private Sub FlatButton1_Click(sender As Object, e As EventArgs) ...
Private Sub FlatButton2_Click(sender As Object, e As EventArgs) ...

Again, same thing with the names of your methods, neither one of them are very meaningful. Now, on to what actually matters:
https://docs.microsoft.com/en-us/windows...tracticona

You are not listening to the return code for this API and after using said API, you blindly continue using the handle without even knowing whether the API call was successful or not. You just catch them all as shown in the code in your FlatButton2_Click method or offer no valuable feedback to the user as shown in the code from the FlatButton1_Click method:

Code

       If txtIconFile.Text.Length > 0 Then
           Dim filePath As String = txtIconFile.Text
           Dim iconLarge As IntPtr

           SHDefExtractIcon(filePath, 0, 0, iconLarge, Nothing, 256)

           Dim hiconLarge As Icon = Icon.FromHandle(iconLarge)
           Dim iconLargebmp As Bitmap = hiconLarge.ToBitmap()
           
           If hiconLarge IsNot Nothing Then
               ''#Save it to disk, or do whatever you want with it.
               With SaveFileDialog1
                   .FileName = ""
                   .Filter = "Icon File| *.ico"
                   .ShowDialog()
                   Dim mIcon As New MultiIcon
                   mIcon.Add("iconLarge").CreateFrom(iconLargebmp, IconOutputFormat.Vista)
                   mIcon.SelectedIndex = 0
                   mIcon.Save(.FileName, MultiIconFormat.ICO)

               End With
           End If
       Else
           MessageBox.Show("Choose a file", "Error")
       End If

This is bad practice. Icon.FromHandle can throw exceptions which you're not properly handling. At least catch the exceptions that are useful to your application and make use of meaningful error messages because they're not always "invalid" files that you're catching exceptions for, as you can clearly read on the documentation of the API that you're using. Again, if it's not clear yet: make use of the return values of the API that you're using.

Also, why are you hard-coding the icon size? What would actually be useful or make this a useful application, since this is basically just a wrapper around SHDefExtractIcon, is the ability to let the user extract a custom image size. In addition, you're using the following library in your code base:
https://www.codeproject.com/articles/161...dows-vista

But you only allow .ico files to be saved or utilized. Why is that? Why would you build an entire UI, with a custom theme nonetheless, around an application that can only extract images in 256 pixels and then use an entire library to convert said bitmap to a .ico file while you could have simply used:
https://docs.microsoft.com/en-us/dotnet/...at-ext-3.1

It's weird, because you're already converting the Icon format to a Bitmap in order to display the icon in a PictureBox instance (which, by the way, has an uneventful variable name), so why would you be using said library at all in the first place?

Code

SHDefExtractIcon(filePath, 0, 0, iconLarge, Nothing, 256)
       'If an Icon cannot be made from the file, the file is invalid and another one needs to be chosen
       Try
           Dim TheIcon As Icon = Icon.FromHandle(iconLarge)
           Dim ico As Bitmap = TheIcon.ToBitmap()

           PictureBox1.Image = ico
       Catch
           txtIconFile.Text = ""
           MessageBox.Show("Invalid file, choose a valid exe file", "Error")
       End Try

To me, it looks like a lot of bloat was added to such a simple thing and improper error handling was implemented along with uneventful variable names, whereas you could have been really creative with the tools that you're using. Why not use the image library to its full extent? Maybe a user doesn't just want to save the image as a .ico file, maybe he/she wants to save it as a JPEG or BMP image, for example.

Let's carry on shall we.

Code

File.WriteAllText(.FileName, Resource1.AssemblyInfo.Replace("FILLER_TITLE", txtTitle.Text).Replace("FILLER_DESCRIPTION", txtDescription.Text).Replace("FILLER_COMPANY", txtCompany.Text).Replace("FILLER_PRODUCT", txtProduct.Text).Replace("FILLER_COPY", txtCopyright.Text).Replace("FILLER_TRADEMARK", txtTrademark.Text).Replace("FILLER_COM", txtComVisible.Text).Replace("FILLER_GUID", txtGUID.Text).Replace("FILLER_VERSION", txtAssemblyVersion.Text).Replace("FILLER_LANG", txtNeutralLanguage.Text).Replace("FILLER_FILEVERSION", txtFileVersion.Text))

What the hell is this? Why are you performing all these operations; replace after replace after replace (and on and on) while you could have simply been using String formatting or a StringBuilder, ...? Right now, if I change one of the 'placeholder' values in your resource file, your entire logic is broken and you, as a programmer are forced to fix it and ignorantly lose minutes of your life doing something which could have been prevented if only you had done it properly.

Additionally, if the WriteAllText method fails and since you're not listening for any meaningful exceptions, your entire application simply crashes. You might want to avoid that. Again, do not catch them all, simply catch the exceptions that are relevant to the operation that you're performing and don't go overboard.

By the way, if you're a fan of package managers and don't want to deal with all that additional code of IconLib inside your Git repository, you could install and use this 'unofficial' NuGet package:
https://www.nuget.org/packages/IconLib.Unofficial

It's not like that library is in active development anyway, and I seriously question your usage of such an outdated library in the first place. Hell, I question your usage of this library because you're not even using 10% of it's capabilities (figure of speech, you're probably using 3% of its capabilities, but 99% of all statistics are made up anyway).

[Logik]

Hi, I really appreciate your insightful feedback, very useful.

But you only allow .ico files to be saved or utilized. Why is that? Why would you build an entire UI, with a custom theme nonetheless, around an application that can only extract images in 256 pixels and then use an entire library to convert said bitmap to a .ico file while you could have simply used:
https://docs.microsoft.com/en-us/dotnet/...at-ext-3.1

I used this library because the methods in the .NET framework do not save icons in high resolution. They only use 16 colours which end up in really bad looking icons. I know it is overkill, but I just wanted a method that worked with minimal effort 😆. But, I will strip down the library so it only includes code that's useful for my needs.

To me, it looks like a lot of bloat was added to such a simple thing and impropererror handling was implemented along with uneventful variable names, whereas you could have been really creative with the tools that you're using. Why not use the image library to its full extent? Maybe a user doesn't just want to save the image as a .ico file, maybe he/she wants to save it as a JPEG or BMP image, for example.

This is a feature that I am still implementing. Letting the user create their own icons from image formats like PNG or JPEG or save icons to other image formats  will be really easy to include. Also, I'm still working on letting the user change icons in existing execuables to an icon of their choosing.

Once again, thanks for taking the time to go over my code and provide feedback!

[Dredd]

-snip-

Nice level of effort but give the guy a break, giving tips and advice is one thing but doing it in a needlessly sarcastic and arrogant manner is not the way to go about it. Someone might of dunked their balls in your coffee this morning but don't take it out on others.

[ViRusRat]

There are certainly some comments that can be made about the state of your Git repository. First of all, why are these folders and data in said folder a thing?

https://github.com/LogikError/Icon-and-A.../obj/Debug
https://github.com/LogikError/Icon-and-A.../bin/Debug
https://github.com/LogikError/Icon-and-A...conLib/obj

These do not belong in a Git repository or on GitHub in general.

Second of all, the name Form1.vb doesn't really mean a whole lot. You should take proper care of your variable- and file names and actually give them meaningful names. That's not the only place where you don't use proper names;

Code

Private Sub FlatButton1_Click(sender As Object, e As EventArgs) ...
Private Sub FlatButton2_Click(sender As Object, e As EventArgs) ...

Again, same thing with the names of your methods, neither one of them are very meaningful. Now, on to what actually matters:
https://docs.microsoft.com/en-us/windows...tracticona

You are not listening to the return code for this API and after using said API, you blindly continue using the handle without even knowing whether the API call was successful or not. You just catch them all as shown in the code in your FlatButton2_Click method or offer no valuable feedback to the user as shown in the code from the FlatButton1_Click method:

Code

       If txtIconFile.Text.Length > 0 Then
           Dim filePath As String = txtIconFile.Text
           Dim iconLarge As IntPtr

           SHDefExtractIcon(filePath, 0, 0, iconLarge, Nothing, 256)

           Dim hiconLarge As Icon = Icon.FromHandle(iconLarge)
           Dim iconLargebmp As Bitmap = hiconLarge.ToBitmap()
           
           If hiconLarge IsNot Nothing Then
               ''#Save it to disk, or do whatever you want with it.
               With SaveFileDialog1
                   .FileName = ""
                   .Filter = "Icon File| *.ico"
                   .ShowDialog()
                   Dim mIcon As New MultiIcon
                   mIcon.Add("iconLarge").CreateFrom(iconLargebmp, IconOutputFormat.Vista)
                   mIcon.SelectedIndex = 0
                   mIcon.Save(.FileName, MultiIconFormat.ICO)

               End With
           End If
       Else
           MessageBox.Show("Choose a file", "_error_")
       End If

This is bad practice. Icon.FromHandle can throw exceptions which you're not properly handling. At least catch the exceptions that are useful to your application and make use of meaningfulerror messages because they're not always "invalid" files that you're catching exceptions for, as you can clearly read on the documentation of the API that you're using. Again, if it's not clear yet: make use of the return values of the API that you're using.

Also, why are you hard-coding the icon size? What would actually be useful or make this a useful application, since this is basically just a wrapper around SHDefExtractIcon, is the ability to let the user extract a custom image size. In addition, you're using the following library in your code base:
https://www.codeproject.com/articles/161...dows-vista

But you only allow .ico files to be saved or utilized. Why is that? Why would you build an entire UI, with a custom theme nonetheless, around an application that can only extract images in 256 pixels and then use an entire library to convert said bitmap to a .ico file while you could have simply used:
https://docs.microsoft.com/en-us/dotnet/...at-ext-3.1

It's weird, because you're already converting the Icon format to a Bitmap in order to display the icon in a PictureBox instance (which, by the way, has an uneventful variable name), so why would you be using said library at all in the first place?

Code

SHDefExtractIcon(filePath, 0, 0, iconLarge, Nothing, 256)
       'If an Icon cannot be made from the file, the file is invalid and another one needs to be chosen
       Try
           Dim TheIcon As Icon = Icon.FromHandle(iconLarge)
           Dim ico As Bitmap = TheIcon.ToBitmap()

           PictureBox1.Image = ico
       Catch
           txtIconFile.Text = ""
           MessageBox.Show("Invalid file, choose a valid exe file", "_error_")
       End Try

To me, it looks like a lot of bloat was added to such a simple thing and impropererror handling was implemented along with uneventful variable names, whereas you could have been really creative with the tools that you're using. Why not use the image library to its full extent? Maybe a user doesn't just want to save the image as a .ico file, maybe he/she wants to save it as a JPEG or BMP image, for example.

Let's carry on shall we.

Code

File.WriteAllText(.FileName, Resource1.AssemblyInfo.Replace("FILLER_TITLE", txtTitle.Text).Replace("FILLER_DESCRIPTION", txtDescription.Text).Replace("FILLER_COMPANY", txtCompany.Text).Replace("FILLER_PRODUCT", txtProduct.Text).Replace("FILLER_COPY", txtCopyright.Text).Replace("FILLER_TRADEMARK", txtTrademark.Text).Replace("FILLER_COM", txtComVisible.Text).Replace("FILLER_GUID", txtGUID.Text).Replace("FILLER_VERSION", txtAssemblyVersion.Text).Replace("FILLER_LANG", txtNeutralLanguage.Text).Replace("FILLER_FILEVERSION", txtFileVersion.Text))

What the hell is this? Why are you performing all these operations; replace after replace after replace (and on and on) while you could have simply been using String formatting or a StringBuilder, ...? Right now, if I change one of the 'placeholder' values in your resource file, your entire logic is broken and you, as a programmer are forced to fix it and ignorantly lose minutes of your life doing something which could have been prevented if only you had done it properly.

Additionally, if the WriteAllText method fails and since you're not listening for any meaningful exceptions, your entire application simply crashes. You might want to avoid that. Again, do not catch them all, simply catch the exceptions that are relevant to the operation that you're performing and don't go overboard.

By the way, if you're a fan of package managers and don't want to deal with all that additional code of IconLib inside your Git repository, you could install and use this 'unofficial' NuGet package:
https://www.nuget.org/packages/IconLib.Unofficial

It's not like that library is in active development anyway, and I seriously question your usage of such an outdated library in the first place. Hell, I question your usage of this library because you're not even using 10% of it's capabilities (figure of speech, you're probably using 3% of its capabilities, but 99% of all statistics are made up anyway).

👍👍👍👍👍👍👍👍👍👍👍💯💯💯