HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Is this common way to build this class and method?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thismethodwaycommonandclassbuild

Problem

I am a new developer therefore I have many questions:

In this class I get file path on the disk (wireshark file that needs to be converted to pcap format extension) and convert it with the method convertFileToPcap(), in the constructor I am calling convertFileToPcap() and from the main after create the Editcap object I am receiving the new file path (pcap format) with the property getNewFileName() who return _newFileName (class member) and I want to know if there is a better way or appropriate way to do it.

public class Editcap
{
    #region members
    private string _editpcap;
    private ProcessStartInfo _editpcapProcess;
    private FileInfo _fileInfo;
    private string _newFileName;
    #endregion

    #region c'tor
    public Editcap(FileInfo fileinfo)
    {
        _fileInfo = fileinfo;
        _newFileName = "";
        convertFileToPcap();
    }
    #endregion

    public void convertFileToPcap()
    {
        string oldFileExtension = _fileInfo.Extension;
        _newFileName = _fileInfo.FullName.Replace(oldFileExtension, "_new") + ".pcap";
        _editpcapProcess = new ProcessStartInfo(string.Format("\"{0}\"", _editpcap));
        _editpcapProcess.Arguments = (string.Format("{2}{0}{2} -F libpcap {2}{1}{2}", _fileInfo.FullName, _newFileName, "\""));
        _editpcapProcess.WindowStyle = ProcessWindowStyle.Hidden;
        _editpcapProcess.RedirectStandardOutput = true;
        _editpcapProcess.RedirectStandardError = true;
        _editpcapProcess.CreateNoWindow = true;
        _editpcapProcess.UseShellExecute = false;
        _editpcapProcess.ErrorDialog = false;
        Process capinfosProcess = Process.Start(_editpcapProcess);
        capinfosProcess.WaitForExit();
    }

    public string getNewFileName()
    {
        return _newFileName;
    }
}

Solution

Hello and welcome to Code Review!

  • I'd like to first direct you to Microsoft's official naming


guidelines for C# development. Your code goes against a few of
those, and if that's your company's or team's guidelines, than great!
If you don't have an official style, start with the ones I linked to.

  • I'd definitely not call the convert method from the constructor.


This could throw exceptions that would prevent the object from being
created. Keep your processing methods separate and let the caller
call it. It IS public, after all!

  • your member variables seem a little too, well, global, in some cases.


Only _fileInfo and _newFileName are used in more than one method.
Keep the others local to their methods.

  • As part of this, note that _editpcap never gets assigned and is


always null. I don't think that's what you're intending.

  • the getNewFileName method looks very Java-like as Java didn't have


the concept of properties. This is a prime candidate for being made a
property.

  • the Process class is IDisposable, so wrapping it in a using


block is idiomatic.

  • now I'm just getting into minor nitpicking, the fields set in the


constructor are invariant after construction, so make the intent
known by using the readonly keyword. Also, you can employ modern C#
amenities as object initializers and the var keyword.

All this being said, here is my proposed refactor of the code:

public class Editcap
{
    #region members
    private readonly FileInfo fileInfo;
    private string newFileName = string.Empty;
    #endregion

    #region c'tor
    public Editcap(FileInfo fileinfo)
    {
        if (fileinfo == null)
        {
            throw new ArgumentNullException("fileInfo");
        }

        this.fileInfo = fileinfo;
    }
    #endregion

    public void ConvertFileToPcap()
    {
        this.newFileName = this.fileInfo.FullName.Replace(this.fileInfo.Extension, "_new") + ".pcap";

        string editpcap = null; // still not set, need to fix!
        var editpcapProcess = new ProcessStartInfo(string.Format("\"{0}\"", editpcap))
        {
            Arguments = string.Format("{2}{0}{2} -F libpcap {2}{1}{2}", this.fileInfo.FullName, this.newFileName, "\""),
            WindowStyle = ProcessWindowStyle.Hidden,
            RedirectStandardOutput = true,
            RedirectStandardError = true,
            CreateNoWindow = true,
            UseShellExecute = false,
            ErrorDialog = false
        };

        using (var capinfosProcess = Process.Start(editpcapProcess))
        {
            capinfosProcess.WaitForExit();
        }
    }

    public string NewFileName
    {
        get
        {
            return this.newFileName;
        }
    }
}

Code Snippets

public class Editcap
{
    #region members
    private readonly FileInfo fileInfo;
    private string newFileName = string.Empty;
    #endregion

    #region c'tor
    public Editcap(FileInfo fileinfo)
    {
        if (fileinfo == null)
        {
            throw new ArgumentNullException("fileInfo");
        }

        this.fileInfo = fileinfo;
    }
    #endregion

    public void ConvertFileToPcap()
    {
        this.newFileName = this.fileInfo.FullName.Replace(this.fileInfo.Extension, "_new") + ".pcap";

        string editpcap = null; // still not set, need to fix!
        var editpcapProcess = new ProcessStartInfo(string.Format("\"{0}\"", editpcap))
        {
            Arguments = string.Format("{2}{0}{2} -F libpcap {2}{1}{2}", this.fileInfo.FullName, this.newFileName, "\""),
            WindowStyle = ProcessWindowStyle.Hidden,
            RedirectStandardOutput = true,
            RedirectStandardError = true,
            CreateNoWindow = true,
            UseShellExecute = false,
            ErrorDialog = false
        };

        using (var capinfosProcess = Process.Start(editpcapProcess))
        {
            capinfosProcess.WaitForExit();
        }
    }

    public string NewFileName
    {
        get
        {
            return this.newFileName;
        }
    }
}

Context

StackExchange Code Review Q#19385, answer score: 6

Revisions (0)

No revisions yet.