Developers.RefactoringGoals
Refactoring Goals
A brainstormed list of goals and guiding principles to follow in refactoring the Shareaza source code.
More encapsulation, less inheritance.
Solve code duplication with a C function.
To be fully object-oriented, Shareaza puts everything in one class or another. Sometimes, this has placed functionality where it doesn't match or belong. The last method in CConnection is StartsWith, which determines if one string begins with another. Other times, this puts exactly the same code in two different classes. CBuffer and CSocket both calculate when they need to resize their buffer to hold more data.
If a bit of code is useful to several classes, it should be OK to put it in a C function, and call that C function from both places. Useful jobs related to text, memory, encoding, and hashing could be presented to the rest of Shareaza as C libraries.
Wrap the platform to fight code duplication.
Right now, there are lots of places where the Shareaza code calls MultiByteToWideChar. Each time, the function is called twice: first to size the required buffer, and then to perform the conversion. Like a lot of Win32 APIs, MultiByteToWideChar has many arguments, most of which are always filled with NULL and 0 defaults.
We should refactor Shareaza to eliminate this code duplication. Instead of calling functions like MultiByteToWideChar all over the place, we could wrap it into one function that uses CBuffer, and just call that.
Make the code quicker to understand, easier to fix, and possible to build more upon.
This is the goal of refactoring. It's like doing a second version not of the program, but of the code.
Each class does exactly one thing.
More and smaller classes.
Do each peer-to-peer job in exactly one place.
Data classes like CBuffer and CHost.
Platform classes like CCompress and CWindowsFirewall.
Separate the networks.
Separate the user interface.
More encapsulation, less inheritance. Convert big inheritance trees like CConnection and long chains like CNeighbours into better encapsulated classes that help and contain each other, but do not inherit from each other nearly as much. This design change would let us pass a pointer to a CConnection object when the handshake is over instead of having to copy-convert a CShakeNeighbour object into a CG1Neighbour object.
Solve code duplication with a C function. Instead of having the same text encoding code all over the place, and instead of calling MultiByteToWideChar twice all over the place, wrap duplicated code like this into C functions. Then, just call the C function. Instead of having the bottom of each class present a makeshift library for that class to use, and have all of these little libraries be very similar (compare the bottom of CBuffer to the bottom of CSocket, for instance), write them into one good library that is separate and that all of Shareaza can use.
Easier to understand, fix, build more upon. This is the purpose of refactoring. It may seem like programming in circles to rewrite a section of code so that it does exactly the same thing as before. But, if it is faster for a developer to understand, easier for a developer to debug, and again simple enough to build the complexity of new features upon, then it's worth it.
Each class does exactly one thing. Now, lots of classes do 2 or 3 things. CConnection, for instance, holds a socket and meters bandwidth. Each class should only do one thing, and it should be the only class that does it. This would break a class for the metering and a class for the socket out of CConnection, for instance.
More and smaller classes. Imagine you are seeing the source code of two new programs for the first time. One has 300 classes that each do 3 things. The other has 900 classes that each do exactly one job. It will be easier to understand the 900-class code.
What does this class do? in a single-line comment. We should be able to explain what each class does in a single-line comment. The way multiple jobs are organized into a single class now, and the way now that classes inherit from one another in long chains, makes this difficult.
Do each peer-to-peer job in exactly 1 place. If you know a Gnutella program negotiates the handshake, and you're looking at the Shareaza code for the first time, it's difficult to find where the code is that does the handshake. Is it CHandshake, CHandhsakes, or CShakeNeighbour? Then, when you find out handshakes headers are processed in these three classes as well as methods of CConnection and elsewhere, this doesn't make it any better. Each well-understood task a peer-to-peer program has to do should be done completely within a single class with a matching name. Jobs shouldn't be spread across multiple classes.
Give methods unique names, not like Send, Run, and OnRead. So many classes have methods with names like OnRead and OnWrite that it's difficult to figure out where control will jump to next. The only way to be really sure is to step through the code. A text search on Send will pull up all kinds of results that aren't the method you're looking for. We should rename these methods what they actually do, specific to that class and that job. With more unique names, they will be easier for developers to remember and for text searches to find.
Data classes like CBuffer and CHost. Small, fundamental classes that just hold some data in a well-understood format are good. CBuffer already is one in Shreaza. CHost, which would hold an IP address and port number, would be another. A third could be CTick, wrapping a DWORD tick count and having methods like bool Elapsed(time). Little classes like this might seem trivial, but it is much better to wrap their few lines of code into a small class than it is to write them, over and over again, all over the rest of Shareaza.
Platform classes like CZLib and CWindowsFirewall. Let's say you are in the middle of writing Shareaza and you need to compress some data. You could just use zlib directly to get the current job done, and keep going with your task at hand. Or, you could figure out all the things Shareaza will need to do with zlib, and wrap them into a helpful class, presenting exactly what Shareaza needs, and nothing more. In many places, Shareaza has chosen the first option. We should refactor it to the second. This idea is about the Shareaza code not using the platform directly, but wrapping it into platform service classes, and then using those.
Separate the networks. Now, the code for Gnutella, Gnutella 2, and eDonkey2000 is mixed together. You can't really look at just the Gnutella portion of Shareaza to understand how just Gnutella works. You can't change the Gnutella portion without worrying how your changes will affect the other networks. In refactoring Shareaza, we should separate the networks. When looking only at the Gnutella portion of Shareaza, it's design should match that of a Gnutella-only program. It shouldn't be any more complicated.
Separate the user interface. Now, the peer-to-peer networking code for Shareaza is tied to the user interface code. Some data related to networking seems to be held by tabbed windows in the user interface, and packets are shown to windows in a loop. We should isolate the user interface from the networking code completely. It should be possible to just delete some files, and turn Shareaza into a peer-to-peer DLL.