2
\$\begingroup\$

I've written a program which does the following things:

  • calls third-party utility to get mp3 file fingerprint
  • asks MusicBrainz IDs from AcoustID service
  • asks metadata from MusicBrainz for each ID

I've never used networking in C# before, so might have done some mistakes in my code (or some parts of it might be unreliable). That's why I'm posting this.

P.S. I'm targeting .NET 4.0 and prefer to avoid using of 3rd-party external libraries for communicating with remote hosts, deserializing JSON documents, etc.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Diagnostics;
using System.Net;
using System.Net.Sockets;
using System.IO;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Json;

namespace ConsoleApplication16
{
    [DataContract]
    class acoustid_mbids
    {
        [DataContract]
        public class result{
            [DataMember]
            public double score { get; set; }
            [DataMember]
            public string id { get; set; }
            [DataMember]
            public recording[] recordings { get; set; }
        }

        [DataContract]
        public class recording
        {
            [DataMember]
            public string id { get; set; }
        }

        [DataMember]
        public string status { get; set; }
        [DataMember]
        public result[] results { get; set; }
    }

    [DataContract]
    class recording
    {
        [DataMember]
        public string title { get; set; }
        [DataMember]
        public string length { get; set; }
        [DataMember(Name = "artist-credit")]
        public artist[] artists { get; set; }
        [DataMember]
        public genre[] genres { get; set; }
        [DataMember]
        public release[] releases { get; set; }

        [DataContract]
        public class artist
        {
            [DataMember]
            public string name;
            [DataMember]
            public string joinphrase;
        }

        [DataContract]
        public class genre
        {
            [DataMember]
            public int count;
            [DataMember]
            public string name;
        }

        [DataContract]
        public class release
        {
            [DataMember]
            public string title;
            [DataMember]
            public string date;
            [DataMember]
            public string id; //required for fetching artwork
        }
    }

    

    class Program
    {
        public static List<recording> avail_recordings = new List<recording>();
        public static List<string> fpcalc_output = new List<string>();
        public const string API_KEY="xxxxxxx";
        public static HttpWebRequest request;
        public static HttpWebResponse response;
        static void Main(string[] args)
        {
            DataContractJsonSerializer parseJson;
            using (Process proc = new Process())
            {
                proc.StartInfo.FileName = "fpcalc.exe";
                proc.StartInfo.Arguments = " Kalimba.mp3";
                proc.StartInfo.RedirectStandardOutput = true;
                proc.StartInfo.UseShellExecute = false;
                proc.OutputDataReceived += new DataReceivedEventHandler(proc_OutputDataReceived);
                proc.Start();
                proc.BeginOutputReadLine();
                proc.WaitForExit();
            }
            ServicePointManager.SecurityProtocol |= (SecurityProtocolType)3072;
            request = (HttpWebRequest)HttpWebRequest.Create("http://api.acoustid.org/v2/lookup?client="+API_KEY+"&duration="+fpcalc_output[0].Replace("DURATION=",String.Empty)+"&fingerprint="+fpcalc_output[1].Replace("FINGERPRINT=",String.Empty)+"&meta=recordingids");
            request.Method = "GET";
            response = (HttpWebResponse)request.GetResponse();
            parseJson = new DataContractJsonSerializer(typeof(acoustid_mbids));
            acoustid_mbids acoustid_session = (acoustid_mbids)parseJson.ReadObject(response.GetResponseStream());
            parseJson = new DataContractJsonSerializer(typeof(recording));
            response.Close();
            response = null;
            for (int i = 0; i < acoustid_session.results.Length; i++)
            {
                for (int j = 0; j < acoustid_session.results[i].recordings.Length; j++)
                {
                    request = (HttpWebRequest)HttpWebRequest.Create("http://musicbrainz.org/ws/2/recording/" + acoustid_session.results[i].recordings[j].id + "?fmt=json&inc=artist-credits+releases+genres");
                    request.KeepAlive = false;
                    request.Method = "GET";
                    request.UserAgent = "SimpleID3Editor/1.0 ( [email protected] )";
                    try
                    {
                        response = (HttpWebResponse)request.GetResponse();
                    }
                    catch (WebException ex)
                    {
                        Console.WriteLine((int)((HttpWebResponse)ex.Response).StatusCode);
                        break;
                    }
                    if (response != null)
                    {
                        avail_recordings.Add((recording)parseJson.ReadObject(response.GetResponseStream()));
                        response.Close();
                    }
                }
            }
            
             
            Console.ReadKey();
        }

        static void proc_OutputDataReceived(object sender, DataReceivedEventArgs e)
        {
            Console.WriteLine("Called!");
            fpcalc_output.Add(e.Data);
            
        }
    }
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Here are my observations:

  • There are couple of unused namespaces
    • right click on the namespaces then select the Remove and Sort Usings menuitem
  • acoustid_mbids \ recording: The general guideline is to prefer Pascal Case for class names.
    • So, AcoustidMbids and Recording would be the suggested names.
    • Please applies these naming for the sub-classes as well.
  • DataMember: This attribute can be really useful when you specify its Name property as well.
    • With that you can separete the naming of the json elements from your application logic:
[DataContract(Name = "acoustid_mbids")]
class AcoustidMbids
{
    [DataContract(Name = "result")]
    public class Result
    {
        [DataMember(Name ="score" )]
        public double Score { get; set; }
        [DataMember(Name = "id")]
        public string Id { get; set; }
        [DataMember(Name = "recordings")]
        public Recording[] Recordings { get; set; }
    }
    
    ...
}
  • Main: Please try to avoid to put everything into the Main method.
    • Try to split into smaller chunks, like this:
static void Main(string[] args)
{
    List<string> fpcalc_output = GetMetaFromMP3("Kalimba.mp3");
    AcoustidMbids acoustid_session = GetMetaFromAcoustid(fpcalc_output);
    List<Recording> avail_recordings = GetMetaFromMusicbrainz(acoustid_session);
}
  • static HttpWebRequest request: Please try to avoid to reuse HttpWebRequest and HttpWebResponse objects.
    • Each method (GetMetaFromAcoustid and GetMetaFromMusicbrainz) should take care of their own instances.
  • proc.StartInfo.FileName = "fpcalc.exe";: You can separate process and its ProcessStartInfo setup.
    • This might increase legibility:
var processInfo = new ProcessStartInfo
{
    FileName = "fpcalc.exe",
    Arguments = " Kalimba.mp3",
    RedirectStandardOutput = true,
    UseShellExecute = false,
};

using (Process proc = Process.Start(processInfo))
{
    proc.OutputDataReceived += new DataReceivedEventHandler(proc_OutputDataReceived);
    proc.BeginOutputReadLine();
    proc.WaitForExit();
}
  • new DataReceivedEventHandler(proc_OutputDataReceived): You can make use of the Lambda expressions to create an anonymous delegate:
proc.OutputDataReceived += (s, e) =>
{
    Console.WriteLine("Called!");
    fpcalc_output.Add(e.Data);
};
  • (SecurityProtocolType)3072: Please try to avoid using magic numbers and use constants instead.
    • Tls12 is not part of the .Net Framework 4.0 (it was introduced in .NET 4.5)
const int Tls12 = 3072;
...
ServicePointManager.SecurityProtocol |= (SecurityProtocolType)Tls12;
  • request = (HttpWebRequest)HttpWebRequest.Create(: As I stated earlier please try to use seperate instances for each call.
    • GET is the default method so you don't have to specify that:
var request = (HttpWebRequest)WebRequest.Create(url);
  • "&duration=" + fpcalc_output[0].Replace("DURATION=", String.Empty):
    • Please try to separate data retrieval logic from url creation:
var duration = fpcalc_output[0].Replace("DURATION=", string.Empty);
var fingerprint = fpcalc_output[1].Replace("FINGERPRINT=", string.Empty);
var url = "http://api.acoustid.org/v2/lookup?client=" + API_KEY + "&duration=" + duration + "&fingerprint=" + fingerprint + "&meta=recordingids";
  • Please try to prefer string.Empty over String.Empty. Please read this paranoid coding article to better understand the difference.
  • Please also bear in mind fpcalc_output[1] can throw OutOfRangeException if it does not have that many element in the collection.
    • Try to use some parsing logic to make your code more robust:
string durationKey = "DURATION="
int durationIdx = fpcalc_output.FindIndex(a => a.Contains(durationKey));
if(durationIdx == -1) throw new InvalidOperationException("Duration is missing from the fpcalc result"); 
fpcalc_output[durationIdx].Replace(durationKey, string.Empty);
  • You should also consider to use some built-in utility to construct QueryParameters, like: System.Web.HttpUtility.ParseQueryString(Related SO topic)
  • response = (HttpWebResponse)request.GetResponse(): Before you perform any further action, please check the StatusCode of the response.
    • If it is different than the expected then web api call most probably failed so it does not make sense to run your code futher.
response = (HttpWebResponse)request.GetResponse();
if(response.StatusCode != HttpStatusCode.OK)
    throw new InvalidOperationException("acoustid did not respond in the expected way.");
  • response.GetResponseStream(): Please bear in mind that here you are using a stream. Which should be disposed if there is no further need for that. Related SO topic
  • parseJson.ReadObject: This can throw SerializationException if the service response differs from the expected structure. Prepare for this case as well to make your solution more robust.
  • for (int i = 0; i < acoustid_session.results.Length; i++): Prefer foreach over for loop (this could highly improve legibility:
foreach (var result in acoustid_session.results)
foreach (var record in result.Recordings)
{
    ...
}
  • response.Close(): As said earlier please prefer using over explicit call of Close.
\$\endgroup\$
3
\$\begingroup\$

Some quick remarks:

  • Properly name things. avail_recordings and fpcalc_output do not follow the Microsoft guidelines (e.g. don't use "snake case", don't needlessly abbreviate,...).

  • request = (HttpWebRequest)HttpWebRequest.Create("http://api.acoustid.org/v2/lookup?client="+API_KEY+"&duration="+fpcalc_output[0].Replace("DURATION=",String.Empty)+"&fingerprint="+fpcalc_output[1].Replace("FINGERPRINT=",String.Empty)+"&meta=recordingids"); is nearly impossible to read.

    Consider using something like new List<KeyValuePair<string, string>> and store each key/value pair of the query string as a KeyValuePair<string, string> and then compile the query string from that collection with .Select(x => string.Format("{0}={1}", x.Key, x.Value)) or use a NameValueCollection.

  • Split your code into smaller methods/classes which do a single thing. Have one method that calls fpcalc, another that calls acoustid.org, another that calls musicbrainz.org, etc.

  • Considering that you never use i and j except to access a particular item in a collection, why don't you use foreach?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.