Code Review Request - Feedback/Suggestions Wanted

[G_Hosa_Phat]

Hello, all. I'm currently working on trying to build a somewhat "generic" class for interacting with web-based API's. I'm working on a premise that the API's this will work with return JSON structures that I can deserialize using Newtonsoft's JSON.NET library. Since I know that each API will return different JSON structures, I've built response classes for most of the API's I'm currently working with.

As you can see in the code listings below, there are a few helper methods (extension methods, etc.) and other referenced classes that I'm not explicitly including here to try to prevent this post from getting out of control (I can provide any of that code if you really want to see it). Also, I've tried to obfuscate a few details here and there, so you certainly may find the occasional typo. Still, with the production code, everything here seems to be working as expected. I'm just trying to make it "better".

I do realize it's a pretty big ask to go over all of this, so I understand completely if you just ignore this post and move on. It's just that I feel like I've taken this about as far as I can and, since I'm the only developer/programmer I personally know, I really want to run this through someone else's filters before going too much further.

Imports System.Net
Imports System.ComponentModel
Imports Newtonsoft.Json
 
Public MustInherit Class APIBase
    Protected Friend Enum WebMethod
        <Description("GET")> HTTPGET = 1
        <Description("POST")> HTTPPOST = 2
        <Description("PUT")> HTTPPUT = 3
        <Description("DELETE")> HTTPDELETE = 4
    End Enum
 
    Protected Property API_URL As String
 
    Protected Function SubmitAPIRequest(ByVal APIURL As String, ByVal Method As WebMethod, ByVal APIEndpoint As String,
                                        Optional ByVal RequestParameters As List(Of String) = Nothing,
                                        Optional ByVal BodyContentType As String = "text/plain, application/json",
                                        Optional ByVal SubmitBody As Object = Nothing,
                                        Optional ByVal AdditionalHeaders As WebHeaderCollection = Nothing,
                                        Optional ByVal Protocol As SecurityProtocolType = SecurityProtocolType.Tls12,
                                        Optional ByVal IgnoreCertificateErrors As Boolean = True) As String
 
        Dim JSONResponse As String = String.Empty
 
        If APIURL Is Nothing OrElse String.IsNullOrEmpty(APIURL) Then
            Throw New ArgumentNullException(NameOf(APIURL), "A valid API URL is required")
        End If
 
        Me.API_URL = APIURL
 
        If APIEndpoint IsNot Nothing AndAlso Not String.IsNullOrEmpty(APIEndpoint) Then
            Dim APIRequest As HttpWebRequest = Nothing
            Dim RequestURL As String = Me.API_URL & APIEndpoint
 
            Try
                If Not RequestParameters Is Nothing Then
                    If RequestParameters.Count > 0 Then
                        RequestURL = RequestURL & "?"
 
                        For Each Parameter As String In RequestParameters
                            RequestURL = RequestURL & Parameter.ToString & "&"
                        Next Parameter
                    End If
                End If
 
                RequestURL = RequestURL.TrimEnd("&"c)
                APIRequest = WebRequest.Create(RequestURL)
                ServicePointManager.SecurityProtocol = Protocol
 
                With APIRequest
                    Dim IPBuffer As IPAddress = Nothing
 
                    If IPAddress.TryParse(APIRequest.RequestUri.Host, IPBuffer) Then
                        If IPBuffer.IsPrivate Then
                            .ServerCertificateValidationCallback = New Security.RemoteCertificateValidationCallback(AddressOf IgnoreCertificateErrorsOnCallback)
                        End If
                    End If
 
                    .Method = Method.GetDescription
                    .ContentType = BodyContentType
                    .Accept = "text/plain, application/json"
                    .PreAuthenticate = True
                    .Headers("Accept-Encoding") = "text/plain, application/json"
                    .Headers("Charset") = "UTF-8"
 
                    If AdditionalHeaders IsNot Nothing Then
                        For I As Integer = 0 To AdditionalHeaders.Count - 1
                            Dim HeaderKey As String = AdditionalHeaders.GetKey(I)
                            Dim HeaderValues As String() = AdditionalHeaders.GetValues(I)
 
                            .Headers(HeaderKey) = String.Join(", ", HeaderValues)
                        Next
                    End If
 
                    If SubmitBody IsNot Nothing Then
                        Dim BodyBytes As Byte() = Nothing
 
                        If TypeOf SubmitBody Is String Then
                            If Not String.IsNullOrEmpty(SubmitBody.ToString) Then
                                BodyBytes = Text.Encoding.UTF8.GetBytes(SubmitBody.ToString)
                            End If
                        ElseIf TypeOf SubmitBody Is Byte() Then
                            .ContentType = "application/octet-stream"
                            .Accept = "application/octet-stream"
                            BodyBytes = SubmitBody
                        ElseIf TypeOf SubmitBody Is IO.FileInfo Then
                            Dim SubmitFile As IO.FileInfo = CType(SubmitBody, IO.FileInfo)
 
                            Dim ContentMIMEType As String = Utility.MIME.GetMIMETypeFromFile(SubmitFile)
 
                            .ContentType = ContentMIMEType
                            .Accept = ContentMIMEType
 
                            Using FileStream As New IO.FileStream(CType(SubmitBody, IO.FileInfo).FullName, IO.FileMode.Open)
                                Using APIStream As IO.Stream = APIRequest.GetRequestStream()
                                    FileStream.CopyTo(APIStream)
                                End Using
                            End Using
                        End If
 
                        If BodyBytes IsNot Nothing Then
                            APIRequest.ContentLength = BodyBytes.Length
 
                            Using APIStream As IO.Stream = APIRequest.GetRequestStream()
                                APIStream.Write(BodyBytes, 0, BodyBytes.Length)
                                APIStream.Close()
                            End Using
                        End If
                    End If
                End With
 
                Using APIResponse As HttpWebResponse = APIRequest.GetResponse
                    Using APIReader As New IO.StreamReader(APIResponse.GetResponseStream)
                        JSONResponse = APIReader.ReadToEnd
                        APIReader.Close()
                    End Using
                End Using
            Catch wex As WebException
                If wex.Message.ToUpper.Contains("UNAUTHORIZED") Then
                    JSONResponse = "NOT AUTHORIZED - " & wex.Message
                End If
            Finally
                If APIRequest IsNot Nothing Then
                    APIRequest = Nothing
                End If
 
                GC.Collect()
                GC.WaitForPendingFinalizers()
                GC.Collect()
            End Try
        Else
            Throw New ArgumentNullException(NameOf(APIEndpoint), "A valid API endpoint is required")
        End If
 
        Return JSONResponse
    End Function
 
    <DebuggerStepThrough>
    Private Function IgnoreCertificateErrorsOnCallback(ByVal sender As Object, ByVal certificate As System.Security.Cryptography.X509Certificates.X509Certificate, ByVal chain As System.Security.Cryptography.X509Certificates.X509Chain, ByVal sslPolicyErrors As System.Net.Security.SslPolicyErrors) As Boolean
        Return True
    End Function
End Class

And, an example API-specific class inherited from that base:

Imports System.Web
Imports System.Net
Imports System.Text
Imports System.ComponentModel
Imports Newtonsoft.Json
Imports MyApp.Common
Imports MyApp.Common.JSON
 
Public Class SonicWallAPI
    Inherits APIBase
 
    Public Enum SonicOSVersion
        <Description("https://{0}:{1}/api/sonicos/")> Gen6
        <Description("https://{0}:{1}/api/sonicos/")> Gen7
    End Enum
 
    Private Const SonicOSFTPURI As String = "ftp://{0}:{1}@{2}/"
 
    Private LoggedIn As Boolean = False
    Private APIHeaders As WebHeaderCollection
    Private APIEndpoint As String = String.Empty
    Private JSONResponse As String = String.Empty
 
    Protected Property AuthorizationString As String
 
    Private ReadOnly APIVersion As SonicOSVersion
    Private ReadOnly FTPServer As FileTransferServerSettings = ApplicationSettings.CertifyToolsConfiguration.CertifyTools.FTPServer
    Private ReadOnly PFXCredentials As NetworkCredential
    Private ReadOnly BaseURL As String
 
    Public Sub New(ByVal SonicOSHost As String, ByVal SonicOSPort As Integer, ByVal SonicOSCredentials As NetworkCredential,
                   Optional ByVal Version As SonicOSVersion = SonicOSVersion.Gen6,
                   Optional ByVal PFXCredential As NetworkCredential = Nothing,
                   Optional ByVal FTPSettings As FileTransferServerSettings = Nothing)
        MyBase.New
 
        If Version <> SonicOSVersion.Gen6 AndAlso Version <> SonicOSVersion.Gen7 Then
            Throw New ArgumentException("Invalid API version selected", NameOf(Version))
        End If
 
        Me.LoggedIn = False
        Me.APIVersion = Version
        Me.BaseURL = Me.APIVersion.GetDescription
        Me.API_URL = String.Format(BaseURL, SonicOSHost, SonicOSPort.ToString)
        Me.APIHeaders = New WebHeaderCollection
        Me.FTPServer = FTPSettings
 
        If PFXCredential IsNot Nothing Then
            Me.PFXCredentials = New NetworkCredential(PFXCredential.Username, PFXCredential.Password)
        End If
 
        Me.AuthorizationString = "Basic " + Convert.ToBase64String(Encoding.Default.GetBytes(SonicOSCredentials.Username + ":" + SonicOSCredentials.Password))
        Me.APIHeaders.Add("Authorization", Me.AuthorizationString)
    End Sub
 
    ''' <summary>
    ''' Log in and create a new session on the SonicWALL appliance
    ''' </summary>
    Public Sub Login()
        Me.APIEndpoint = "auth"
        Me.JSONResponse = SubmitAPIRequest(Me.API_URL, WebMethod.HTTPPOST, Me.APIEndpoint, AdditionalHeaders:=Me.APIHeaders)
 
        If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
            Dim Authorization As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
            If Authorization.Status.Success Then
                Me.LoggedIn = True
            Else
                Me.LoggedIn = False
                Throw New Exception("Login to SonicWALL appliance failed")
            End If
        Else
            Me.LoggedIn = False
            Throw New Exception(Me.JSONResponse)
        End If
    End Sub
 
    ''' <summary>
    ''' Log out of the current session on the SonicWALL appliance
    ''' </summary>
    Public Sub Logout()
        Me.APIEndpoint = "auth"
        Me.JSONResponse = SubmitAPIRequest(Me.API_URL, WebMethod.HTTPDELETE, Me.APIEndpoint, AdditionalHeaders:=Me.APIHeaders)
 
        If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
            Dim Authorization As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
            If Authorization.Status.Success Then
                Me.LoggedIn = False
            End If
        End If
    End Sub
 
    ''' <summary>
    ''' Import a new SSL certificate from a PFX file provided by the Certificate Authority
    ''' </summary>
    ''' <param name="PFXFile">The PKCS12 file provided by the Certificate Authority containing the SSL certificate to be installed</param>
    ''' <returns></returns>
    Public Function ImportCertificate(ByVal PFXFile As IO.FileInfo) As Boolean
        If Me.LoggedIn Then
            Dim FriendlyName As String = $"SSL Cert Issued {PFXFile.LastWriteTimeUtc.ToString("yyyy-MM-dd")}"
            
            If Me.APIVersion = SonicOSVersion.Gen6 Then
                Me.APIEndpoint = "certificates/import/cert-key-pair/name/{0}/password/{1}"
            Else
                Me.APIEndpoint = "import/certificates/cert-key-pair/name/{0}/password/{1}"
            End If
 
            If Me.FTPServer IsNot Nothing AndAlso Me.FTPServer.FTPServerHost IsNot Nothing AndAlso Not String.IsNullOrEmpty(Me.FTPServer.FTPServerHost) Then
                Dim FTPServerIP As IPAddress = Nothing
 
                IPAddress.TryParse(Me.FTPServer.FTPServerHost, FTPServerIP)
 
                If FTPServerIP Is Nothing Then
                    IPAddress.TryParse(Dns.GetHostEntry(FTPServer.FTPServerHost).AddressList(0).ToString, FTPServerIP)
                End If
 
                If FTPServerIP IsNot Nothing Then
                    Dim FTPURL As String = String.Format(SonicOSFTPURI,
                                                         Me.FTPServer.FTPUser.Username,
                                                         Me.FTPServer.FTPUser.Password,
                                                         FTPServerIP.ToString) & PFXFile.Name
 
                    Me.APIEndpoint += "/ftp/{2}"
 
                    If Utility.FTP.FTPUpload(Me.FTPServer.FTPServerHost, Me.FTPServer.FTPUser, PFXFile) Then
                        Me.JSONResponse = SubmitAPIRequest(Me.API_URL,
                                                           WebMethod.HTTPPUT,
                                                           String.Format(Me.APIEndpoint,
                                                                         HttpUtility.UrlEncode(FriendlyName),
                                                                         HttpUtility.UrlEncode(Me.PFXCredentials.Password),
                                                                         HttpUtility.UrlEncode(FTPURL)),
                                                           AdditionalHeaders:=Me.APIHeaders)
 
 
                        If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
                            Dim APISuccess As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
                            If APISuccess.Status.Success Then
                                Return SetActiveSSLCertificate(FriendlyName)
                            End If
                        End If
                    End If
                End If
            Else
                Dim MIMEType As String = "application/octet-stream"
 
                If Me.APIVersion <> SonicOSVersion.Gen6 Then
                    MIMEType = Utility.MIME.GetMIMETypeFromFile(PFXFile)
                End If
 
                Me.JSONResponse = SubmitAPIRequest(Me.API_URL,
                                                   WebMethod.HTTPPUT,
                                                   String.Format(Me.APIEndpoint,
                                                                 HttpUtility.UrlEncode(FriendlyName),
                                                                 HttpUtility.UrlEncode(Me.PFXCredentials.Password)),
                                                   SubmitBody:=PFXFile,
                                                   BodyContentType:=MIMEType,
                                                   AdditionalHeaders:=Me.APIHeaders)
                If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
                    Dim APISuccess As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
                    If APISuccess.Status.Success Then
                        Return SetActiveSSLCertificate(FriendlyName)
                    End If
                End If
            End If
        Else
            Login()
            Return ImportCertificate(PFXFile)
        End If
 
        Return False
    End Function
 
    ''' <summary>
    ''' Specifies the active SSL certificate for the SonicWALL appliance to use
    ''' </summary>
    ''' <param name="FriendlyName">The Friendly Name of the certificate to set as active</param>
    ''' <returns></returns>
    Private Function SetActiveSSLCertificate(ByVal FriendlyName As String) As Boolean
        If Me.LoggedIn Then
            Dim GlobalSettings As New SonicWALLAdministrationSettings With {
                    .AdminSettingsGroup = New SonicWALLAdministrationSettings.AdminSettings With {
                        .WebManagementConfiguration = New SonicWALLAdministrationSettings.AdminSettings.WebManagementSettings With {
                            .Certificate = New SonicWALLAdministrationSettings.AdminSettings.WebManagementSettings.SSLCertificate With {
                                .FriendlyName = FriendlyName
                            }
                        }
                    }
                }
 
            Me.APIEndpoint = "administration/global"
            Me.JSONResponse = SubmitAPIRequest(Me.API_URL,
                                               WebMethod.HTTPPUT,
                                               Me.APIEndpoint,
                                               SubmitBody:=JsonConvert.SerializeObject(GlobalSettings, ApplicationSettings.JSONSettings),
                                               AdditionalHeaders:=Me.APIHeaders)
 
            If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
                Dim APISuccess As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
                If APISuccess.Status.Success Then
                    Return CommitPendingChanges()
                Else
                    Return False
                End If
            Else
                Return False
            End If
        Else
            Login()
            Return SetActiveSSLCertificate(FriendlyName)
        End If
    End Function
 
    ''' <summary>
    ''' Commits any pending changes to the SonicWALL appliance's configuration
    ''' </summary>
    ''' <param name="RestartDevice">Whether or not the SonicWALL appliance should be restarted after a successful commit</param>
    ''' <returns></returns>
    Private Function CommitPendingChanges(Optional ByVal RestartDevice As Boolean = False) As Boolean
        If Me.LoggedIn Then
            Me.APIEndpoint = "config/pending"
            Me.JSONResponse = SubmitAPIRequest(Me.API_URL, WebMethod.HTTPPOST, Me.APIEndpoint, AdditionalHeaders:=Me.APIHeaders)
 
            If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
                Dim Authorization As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
                Return Authorization.Status.Success
            Else
                Return False
            End If
        Else
            Login()
            Return CommitPendingChanges(RestartDevice)
        End If
    End Function
 
    ''' <summary>
    ''' Restart the SonicWALL appliance at the specified day/time
    ''' </summary>
    ''' <param name="RestartTime"></param>
    ''' <returns></returns>
    Friend Function ScheduleRestart(ByVal RestartTime As Date) As Boolean
        If Me.LoggedIn Then
            Dim AT_TIME As String = RestartTime.ToString("yyyy:MM:dd:HH:mm:ss")
 
            Me.APIEndpoint = $"restart/at/{AT_TIME}"
            Me.JSONResponse = SubmitAPIRequest(Me.API_URL, WebMethod.HTTPPOST, Me.APIEndpoint, AdditionalHeaders:=Me.APIHeaders)
 
            If Not Me.JSONResponse.StartsWith("NOT AUTHORIZED") Then
                Dim Authorization As SonicWALLAPIResponse = JsonConvert.DeserializeObject(Of SonicWALLAPIResponse)(Me.JSONResponse)
 
                Return Authorization.Status.Success
            Else
                Return False
            End If
        Else
            Login()
            Return ScheduleRestart(RestartTime)
        End If
    End Function
End Class

And, finally, here is the API-specific response object definition:

Imports Newtonsoft.Json
 
Namespace MyApp.Common.JSON
#Region "STANDARD RESPONSE OBJECT FROM SONICWALL API"
    Public Class SonicWALLAPIResponse
        <JsonProperty("status")>
        Public Property Status As SonicWALLAPIStatus
    End Class
 
    Public Class SonicWALLAPIStatus
        <JsonProperty("success")>
        Public Success As Boolean
 
        <JsonProperty("info")>
        Public Details As List(Of SonicWALLAPIStatusDetail)
    End Class
 
    Public Class SonicWALLAPIStatusDetail
        <JsonProperty("level")>
        Public LogLevel As String
 
        <JsonProperty("code")>
        Public LogCode As String
 
        <JsonProperty("message")>
        Public LogMessage As String
    End Class
#End Region
End Namespace

I think I've made something fairly solid here, but there are a few things I still want to build in. A couple of examples of where I think it's still lacking:

• I want to create a generic API response "wrapper" object that can take the API-specific JSON responses and "merge" them into something that can then be passed back to the derived classes and other methods for reading and processing. I stumbled across this bit of code (C#? JavaScript? I'm not sure.) on StackOverflow that seems to be something like I want for this wrapper, but I've yet to actually implement it.
  
  

  Smth smth = new Smth()
  {
      Something = "dsfdsfdsfs"
  };
  var serializer = new JavaScriptSerializer();
  string json = serializer.Serialize(this.GetResponse(true, smth));
  var response = this.Request.CreateResponse(HttpStatusCode.OK);
  response.Content = new StringContent(json, Encoding.UTF8, "application/json");
  return response;

  Obviously, the Smth class would be my API-specific response object, but I think I'd rather return the fully populated "wrapper" object than an HttpResponseMessage
  
• I also want to create a base exception object/handler that can take either an API-specific JSON response object (and all of its children) or simply another exception (or even just that exception's message) and create something that can be used elsewhere for logging or whatever.

There may be other things I'm missing/overlooking, but this is as far as I've made it so far. Any help, feedback, critiques, and/or suggestions on how to improve this whole mess are definitely welcome and greatly appreciated. Thank you so much for your time.

[Peter Swinkels]

A few things I noticed:

1. There's is no need to give every item in an enumeration a number. Just set the first item to "1" and the rest will be handled automatically.
2. Look up on string interpolation and System.Text.StringBuilder. Those methods should be used rather than the concatenation I see in your code.

[G_Hosa_Phat]

Thank you so much for the feedback! I've taken some elements of this code from some of my older projects when I was still new to a lot of concepts. I've tried to update most of them to use more effective methods, but there are still plenty of opportunities for me to increase my understanding. I'll take a look at some of my string concatenation and see where I can improve things. I truly appreciate the pointers.

[Peter Swinkels]

You're welcome. Your code looks better than a lot of code I come across.

I have two more suggestions:
1. Enumerations support a data type declaration. ("Enum ... As ...")
2. Some parts of your code probably could be shortened using the If() ternary operator. (No, it doesn't have the same issues as vb6's IIf() function btw.)

These go something like:

If .... Then
   Return ...
Else
  Return ...
End If

This probably can be turned into "Return If(..., ..., ...)"

[G_Hosa_Phat]

All right, so here's what I take from your feedback:
1. Since I'm not dealing with extensive enumeration lists in the code above, possibly declare them like "Enum ... As UShort", or something to minimize the memory allocation required. Also, I can probably remove the explicitly defined values I've assigned on both of them.

2. Look over the string concatenation/interpolation bits. Like I said, I've tried to update things to use more current methods - mostly converting to interpolation as much as possible - but I could easily have overlooked a few bits here and there.

3. I see your point about using the ternary operator form in some of the If...Then...Else statements. I actually had them broken out that way intentionally simply because I personally feel it makes it a bit easier to read, but I also realize that it can cause some "clutter" for more seasoned developers/programmers.

I do have to say, though, I really appreciate the compliment. The truth is that I'm entirely self-taught, so I'm kinda "flying by the seat of my pants" on most of this.

[Peter Swinkels]

1. Would it be more than a few bytes? Still you could explicitly declare a type just to be thorough. I believe vb defaults to Integer.
2. Yeah, I know. It's not always easy to stay up to date and to maintain your code.
3. I like to be thorough but try to avoid overly long code where possible.

I am self-taught too. :-)