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 build application that open Wireshark services (Wireshark has several services) in order to to different things on Wireshark files like Edit, change format, statistics etc... each option usually use different service so i want to build my classes with inheritance i was wonder if what i want to do is appropriate way or proper way:

main class WiresharkServices with the following members and method:

public class WiresharkProcesses
{
    protected string _filePath; //the file path who send to Wiresahrk process
    protected string _capinfos;
    protected string _dumpcap;
    protected string _editcap;
    protected string _mergecap;
    protected string _rawshark;
    protected string _text2pcap;
    protected string _tshark;
    protected string _wireshark;

    public void initializeServices()
    {
        if (Directory.Exists(@"C:\Program Files (x86)\Wireshark"))
        {
            _capinfos = @"C:\Program Files (x86)\Wireshark\_capinfos.exe";
            _dumpcap = @"C:\Program Files (x86)\Wireshark\_dumpcap.exe";
            _editcap = @"C:\Program Files (x86)\Wireshark\editcap.exe";
            _mergecap = @"C:\Program Files (x86)\Wireshark\_mergecap.exe";
            _rawshark = @"C:\Program Files (x86)\Wireshark\_rawshark.exe";
            _text2pcap = @"C:\Program Files (x86)\Wireshark\_text2pcap.exe";
            _tshark = @"C:\Program Files (x86)\Wireshark\_tshark.exe";
            _wireshark = @"C:\Program Files (x86)\Wireshark\_wireshark.exe";
        }
        else if (Directory.Exists(@"C:\Program Files\Wireshark"))
        {
            _capinfos = @"C:\Program File)\Wireshark\_capinfos.exe";
            _dumpcap = @"C:\Program Files\Wireshark\_dumpcap.exe";
            _editcap = @"C:\Program Files\Wireshark\editcap.exe";
            _mergecap = @"C:\Program Files\Wireshark\_mergecap.exe";
            _rawshark = @"C:\Program Files\Wireshark\_rawshark.exe";
            _text2pcap = @"C:\Program Files\Wireshark\_text2pcap.exe";
            _tshark = @"C:\Program Files\Wireshark\_tshark.exe";
            _wireshark = @"C:\Program Files\Wireshark\_wireshark.exe";
        }
    }
}

when the application running i am check of course if Wireshark installed on the machine and if not throw exception and in Wireshark exist:

WiresharkServices wservices = new WiresharkServices();
wservices .initializeServices();

and in each class it's own methods.

child class example who receive file path to convert it to other Wireshark format:

public class Editcap : WiresharkProcesses
{
    private string _newFileName;

    public void startProcess(string filePath)
    {
        FileInfo file = new FileInfo(filePath);
        _newFileName = file.FullName.Replace(file.Extension, "_new") + ".pcap";
        ProcessStartInfo editcapProcess = new ProcessStartInfo(string.Format("\"{0}\"", _editcap))
        {
            Arguments = string.Format("{2}{0}{2} -F libpcap {2}{1}{2}", file.FullName, _newFileName, "\""),
            WindowStyle = ProcessWindowStyle.Hidden,
            RedirectStandardOutput = true,
            RedirectStandardError = true,
            CreateNoWindow = true,
            UseShellExecute = false,
            ErrorDialog = false
        };

        using (Process editcap = Process.Start(editcapProcess))
        {
            editcap.WaitForExit();
        }
    }

    public string getNewFileName()
    {
        return _newFileName;
    }
}
share|improve this question
    
You mention that you want to use inheritance but I see no evidence of it. Also, these fields should be properties. You can detect the 32-bit vs the 64-bit once and then build the file paths from that. –  Leonid Dec 11 '12 at 15:56
    
this is the class that all the other classes should inherite, i am checking if Wireshak installed on the machine once the form load. –  user1710944 Dec 11 '12 at 15:58
2  
It's often said that "Inheritance breaks encapsulation" and in this case, that's certainly true. While I can see what you're trying to achieve, it's probably worth looking into using composition rather than inhertiance. From an OOP perspective composition should amost always be favoured. Can you give us an example of a child class that uses this parent? –  Tom B Dec 11 '12 at 16:25
    
child class added –  user1710944 Dec 11 '12 at 16:29
2  

2 Answers 2

up vote 3 down vote accepted

The only thing you are getting out of this inheritance is which directory should be used to access the executable. And you are only using that to define a number of (what should be constant) strings. The fact that you have an error in your value for _capinfos in the seconds case should be an indication that this is not the best way to perform this action.

To accomplish the task of initializing the strings, I would instead do something like this.

public void initializeCommands(String path) {
    _capinfos = path + "_capinfos.exe";
    _dumpcap = path + "_dumpcap.exe";
    _editcap = path + "editcap.exe";
    _mergecap = path + "_mergecap.exe";
    _rawshark = path + "_rawshark.exe";
    _text2pcap = path + "_text2pcap.exe";
    _tshark = path + "_tshark.exe";
    _wireshark = path + "_wireshark.exe";
}

You can wrap this in something that determines the path or even support non-standard paths.

But what this really comes down to is that you are using inheritance just so you have easy access to some constants you might want to use. You are not defining any methods that would specify actual code used between multiple classes that execute Wireshark processes. If Editcap were passed in the path to the executable, it would no longer have a need to the parent class. If the coupling is this loose, inheritance is generally not the best idea.

share|improve this answer
    
OK thank you very much –  user1710944 Dec 11 '12 at 19:04
1  
It would take me too long to rewrite the original code in a way that I consider proper. The biggest problem I see is code repetition, which can be avoided with dictionaries, or you name it. I do have a concern about thread-safety, and everything can be accomplished with static methods, so no actual state is required. As far as improving your answer, you should use a better way to join paths as described here: stackoverflow.com/questions/961704/how-do-i-join-two-paths-in-c –  Leonid Dec 11 '12 at 19:12

This should be a little better, but not perfect. Note that wrote this without having Visual Studio in front of me.

Notice that the WiresharkProcesses class is static, so you cannot inherit from it and you cannot instantiate it. It is like a namespace with a bunch of functions except that the static constructor does some work. This is not perfect. I will gladly accept some suggestions from the community. You could open up some private readonly fields or expose them as properties at least. Notice that repeated access to the properties will cause repeated string concatenation, but it is a very fast operation. Another alternative would be to use auto-properties like public static string CapInfosExePath { get; private set;} instead of the protected fields. Either way, inheritance is not required. The EditCap class has too much boiler-plate code. Frankly, you only need one or two classes altogether, perhaps make them partial and split them into two separate files (just a thought).

using System.IO;

// TODO: also add namespace around this
public static class WiresharkProcesses
{
    // TODO: actually use proper library for this.
    // http://stackoverflow.com/questions/1085584/how-do-i-programmatically-retrieve-the-actual-path-to-the-program-files-folder?lq=1
    // Note that a spanish Windows version will have "C:\Archivos de programa\" instead
    private static readonly string ProgramFiles64Path = @"C:\Program Files";
    private static readonly string ThirtyTwoBitSuffix = @" (x86)";
    private static readonly string ProgramFiles32Path = Path.Join(ProgramFiles64Path, ThirtyTwoBitSuffix);
    private static readonly string WiresharkDirName = @"Wireshark";
    private static bool Is64Bit = true;
    private static string ProgramFilesPath = null;
    private static readonly string WiresharkDirectoryPath = null;

    // Static constructor runs only once
    public static WiresharkProcesses()
    {
        if (Directory.Exists(ProgramFiles64Path))
        {
            Is64Bit = true;
            ProgramFilesPath = ProgramFiles64Path;
        }
        else if (Directory.Exists(ProgramFiles32Path))
        {
            Is64Bit = false;
            ProgramFilesPath = ProgramFiles32Path;
        }
        else
        {
            throw new AppropriateException("WTF!");
        }

        WiresharkDirectoryPath = Path.Combine(ProgramFilesPath, WiresharkDirName);
    }

    private static string GetFullPath(string exeName)
    {
        return Path.Combine(WiresharkDirectoryPath, exeName);
    }

    public static string CapInfosExePath
    {
        get
        {
            // Perhaps this arg should be a variable as well.
            return GetFullPath("_capinfos.exe");
        }
    }

    ...

    public static string WiresharkExePath
    {
        get
        {
            // Perhaps this arg should be a variable as well.
            return GetFullPath("_wireshark.exe");
        }
    }
}
share|improve this answer

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.