5
\$\begingroup\$

I am writing a class which can log the activity of an application during run time. The plan is that the SupportMI string will be sent to a DB, from where I can use some other code to format it properly for readability. I need this class to be usable with .Net-3.5 due to system limitations.

What I need is the method name, the values of the parameters passed, and the output of the method (if specified). I also added a timer class to measure performance:

using System;
using System.Diagnostics;

namespace MyTimer
{
    class MILogger : IDisposable
    {
        private string SupportMI;
        private Stopwatch stopwatch = new Stopwatch();
        private string[] parameters;
        public static string methodoutput;

        public MILogger()
        {
            stopwatch.Start();
        }

        public MILogger(params string[] args)
        {
            stopwatch.Start();
            parameters = args;
        }

        public void Dispose()
        {
            stopwatch.Stop();

            // New stacktrace
            StackTrace stackTrace = new StackTrace();

            // Method
            var method = stackTrace.GetFrame(1).GetMethod();
            string methname = method.Name;
            SupportMI += methname + "(";

            // Sorting parameters
            for (int i = 0; i < parameters.Length; i++)
            {
                if (i > 0)
                {
                    SupportMI += ",";
                }

                SupportMI += parameters[i];
            }

            // Timer, methooutput and some punctuation
            SupportMI += ") { ";
            SupportMI += methodoutput;
            SupportMI += "   TIMER: " + stopwatch.ElapsedMilliseconds.ToString();
            SupportMI += "}";
        }

        public static void SendMI()
        {
            // Send SupportMI to a database
            DBConnection db = new DBConnection(SupportMI);
        }

    }
}

Usage:

class Program
{
    static void Main(string[] args)
    {
        TestMethod("HELLO", "HI", 123);

        // Other code

        MILogger.SendMI();
    }

    public static string TestMethod(string str, string qwe, int dsad)
    {
        using (new MILogger(str, qwe, dsad.ToString()))
        {
            MILogger.methodoutput = str + qwe + dsad.ToString();
            return str + qwe + dsad.ToString();                
        }
    }
}

And the output looks like this:

TestMethod(HELLO,HI,123) { HELLOHI123   TIMER: 1}

With this I can more easily debug instances of the application being used by checking that the correct parameters are being passed in each method.

I thought it was weird that there is no easier way to get the parameter values. Does anyone have any feedback or thoughts on how this could be done better?

I'm also aware that I've used multiple ToStrings()s where this may not be strictly necessary.

The DB class:

class DBConnection
{
    public DBConnection(string objecttosend)
    {
        // connect to DB
        using (SqlConnection conn = new SqlConnection())
        {
            // create the connection string
            conn.ConnectionString = "ConnectionStringGoesHere";

            // open the db connection
            conn.Open();

            // insert info into DB
            SqlCommand insertCmd = new SqlCommand("INSERT INTO IncidentLog (SupportMI) VALUES (@SupportMI)", conn);

            insertCmd.Parameters.Add(new SqlParameter("@SupportMI", objecttosend));

            // execute
            insertCmd.ExecuteNonQuery();

            // close the db connection
            conn.Close();
        }
    }
}

Example output of what I'm going for (achieved with a lot of hard-coding):

INITIALISE 
{
    Servers.GetServers()
    {
    }
    GetSession()
    {
        CheckFile()
        {
            [server file exists] Query(XA7-02)
            {
                StdOut(QUERY SESSION HelenA /SERVER:XA7-02.mydomain.co.uk) 
                {
                    ExecuteCommand() 
                    {
                    }
                    SESSIONNAME       USERNAME                 ID  STATE   TYPE        DEVICE  ica-cgp#0         User.Name                    2  Active                      
                }
            }
            [correct server]
        }
    }
}
INIT    1953    GetProcList(XA7-02)
{
    StdOutAdminList(tasklist /S XA7-02.mydomain.co.uk /FI "SESSION eq 2" /FO CSV /NH)
    {
        ExecuteListCommand() 
        {
        }
    }
    Creating Proc LYNC.EXE--addingLYNC.EXE--Creating Proc BUSYLIGHT.EXE--addingBUSYLIGHT.EXE--Creating Proc OUTLOOK.EXE--addingOUTLOOK.EXE--Creating Proc SLLAUNCHER.EXE--addingSLLAUNCHER.EXE--Creating Proc TOTALSPEECH.EXE--addingTOTALSPEECH.EXE--Creating Proc PROWC.EXE--addingPROWC.EXE--
}
Kill()
{
    ExecAdmin(taskkill /S XA7-02 /PID 6476 /F) 
    {
    }
}
| EXIT |
\$\endgroup\$
  • \$\begingroup\$ Is this in its posted form working ??? The private SupportMI field is constructed but never exposed to the outside, so the posted output won't ever happen. \$\endgroup\$ – Heslacher Feb 8 '16 at 13:43
  • \$\begingroup\$ @Heslacher The string is built, but I haven't included the code for the database connection, as I felt this was not really part of the problem. Is that what you mean by exposed to the outside? It works in that the string is built correctly \$\endgroup\$ – Bassie Feb 8 '16 at 13:46
  • \$\begingroup\$ We are reviewing working complete code only. Because you have omitted parts of the code I would tell you some naming issues and that the Dispose() method is superflous because it is effectivly only stoping the StopWatch. So what do you think, how could we review your code in a way which is satisfying for you ?? \$\endgroup\$ – Heslacher Feb 8 '16 at 13:50
  • \$\begingroup\$ But this is the complete code - or do you want me to add the method used for sending to the DB? Or an actual usage example? For all intents and purposes the code is a string-building exercise, but the string must include containing method/parameter details ... I don't understand why Dispose() is superfluous - it is doing a few things after the using{} block isn't it? Or will Dispose() automatically do all these things? \$\endgroup\$ – Bassie Feb 8 '16 at 13:56
  • \$\begingroup\$ "But this is the complete code - or do you want me to add the method used for sending to the DB?" Does this code belong to this class ? If yes this isn't the complete code. If no, then how on earth should the SupportMI field be accessed from outside of this class ? \$\endgroup\$ – Heslacher Feb 8 '16 at 14:00
4
\$\begingroup\$

Naming

Fields should be named using camelCase casing, so SupportMI -> supportMI. Abbreviations shouldn't be used for naming things, but I assume that MI has a special business meaning to you. If not, consider to rename this as well.

If the field is public one should use PascalCase casing for naming it. For compound words each "new" word will start with an uppercase letter , so methodoutput -> MethodOutput.

Using abbreviations for names is a no go as well, so andmethname->methodName`.

General

By using the string.Join() method the "sorting of the parameter" can be improved a lot.

Instead of using string concatenation you should use a StringBuilder and its Append() method. This avoids the creation of a lot of string objects.

The StringBuilder's methods are implemented using a fluent interface which means that the methods return the StringBuilder's instance so one code for instance use it like stringBuilderObject.Append("someValue").Append("someOtherValue)...

You could implement this like so

var method = stackTrace.GetFrame(1).GetMethod();
var builder = new StringBuilder();
builder.Append(method.Name)
       .Append("(")
       .Append(string.Join(",", parameters))
       .Append(") { ")
       .Append(MethodOutput)
       .Append("   TIMER: ")
       .Append(stopwatch.ElapsedMilliseconds)
       .Append("}");  

supportMI = builder.ToString();
\$\endgroup\$
  • 1
    \$\begingroup\$ What about using string.Format method or string interpolation instead of StringBuilder? \$\endgroup\$ – Bogdan Yarema Feb 9 '16 at 10:08
1
\$\begingroup\$

You can greatly improve the usabitlity of this helper if you implement the following suggestions:

  • do not use any static fields, they can be overwritten if you use it a parallel context
  • do not pass strings but objects and let them automatically convert to strings later
  • you can add more info by getting names of the parameters
  • make the Result method return the result so you can assign and return the method result at the same time

Here's an example:

class MILogger<T> : IDisposable
{
    private readonly string _methodName;
    private Stopwatch _stopwatch;
    private IDictionary<string, object> _parameters = new Dictionary<string, object>();
    private T _result;

    public MILogger(params object[] args)
    {
        var stackTrace = new StackTrace();
        var method = stackTrace.GetFrame(1).GetMethod();
        _methodName = method.Name;
        _parameters = 
            method.GetParameters()
            .Select(p => p.Name)
            .Zip(args, (name, value) => new { name, value })
            .ToDictionary(x => x.name, x => x.value);

        _stopwatch = Stopwatch.StartNew();
    }

    public T Result(T result)
    {
        return (_result = result);
    }

    public void Dispose()
    {
        _stopwatch.Stop();

        var parameters = string.Join(", ", _parameters.Select(p 
            => string.Format("{0} = \"{1}\"", p.Key, p.Value)));

        SendMethodInfo(string.Format("{0}({1})", _methodName, parameters));
    }

    private void SendMethodInfo(string methodInfo)
    {
        methodInfo.Dump(); // linqpad
        // ....
    }
}

use:

public static string Foo(string bar, string baz, int qux)
{
    using (var methodLogger = new MILogger<string>(bar, baz, qux))
    {
        return methodLogger.Result(bar + baz + qux);
    }
}

output:

Foo(bar = "HELLO", baz = "HI", qux = "123")

For anonymous-names you can use the metasyntactic variables.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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