Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question

1 Answer 1

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;
        }
    }
}
share|improve this answer
1  
Definitely cleaner, but regions are considered ugly by some plus I do not see any need for saving the state, so a class is only useful as a namespace (and C# does not allow having functions outside of a class). So, I would have no constructor but a single public static method that takes FileInfo and returns a string. That way you get a nice black box. The implementation can be split up amongst several private static methods. –  Leonid Dec 7 '12 at 15:47
    
Yeah, I didn't address the region thing as I have experience inheriting a large codebase from 2003 following the recommended usage then. One hell of a cleanup effort. –  Jesse C. Slicer Dec 7 '12 at 15:56

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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