patterncsharpMinor
Is this common way to build this class and method?
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
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!
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.
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
Only
Keep the others local to their methods.
always
the concept of properties. This is a prime candidate for being made a
property.
block is idiomatic.
constructor are invariant after construction, so make the intent
known by using the
amenities as object initializers and the
All this being said, here is my proposed refactor of the code:
- 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
_editpcapnever gets assigned and is
always
null. I don't think that's what you're intending.- the
getNewFileNamemethod looks very Java-like as Java didn't have
the concept of properties. This is a prime candidate for being made a
property.
- the
Processclass isIDisposable, so wrapping it in ausing
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.