 |
|
|
I'll try and be gentle...
1) C++ classes that have documentation that says things like 'must call X before calling this method' are bad. They're fragile, people will use them wrongly because they can. Most of the time there's no need to build classes that are this fragile. In this particular instance you can remove this problem by using the class's constructor for the purpose that it was intended. That is to construct the class! To do this you move all of the code in GetLocalIP() into the ctor. This means that you can no longer return an error value if the code in GetLocalIP() fails because a ctor cant return an error. To overcome this you should throw an exception on failure. You now have a situation where you can create your object and call all of the methods without needing to document that you have to call GetLocalIP() first. If the class encounters errors during its construction then an exception is thrown and the class never exists so methods can never be called.
2) Multiplexing error returns into return values. Bad programmer. So, how does a client of this class know that the call to the now defunct GetLocalIP() method had failed? He parses it for "Error:" ? Don't return things that can be either a valid result or an error indicator, split it up. In this case take a string by reference to return the ip address in, perhaps, and return a boolean, or an error code.
3) Names of methods. MemberStringToComponents() we're dealing with an IP address, why does a client of the class need to know, or care that this is stored as a string member of the class. A better name might be something like AsComponents(). Likewise, GetHBOIPAddress(), could be AsDWORDInHostByteOrder() or probably something better.... We know it's an IP Address because that's what the class does....
4) Make the class minimal and complete. You probably dont need the host and network byte order methods as winsock provides a conversion that people can use... So you could probably ditch one of them.
5) Know what exists and use it rather than rolling your own. StringToInt(), atoi(), nuf said.
6) m_converter? Hmm, nah. For a start all of the methods should be static as they dont reference any instance data. Then we have the question, why are these better placed in their own class? why not part of the IP address class? And even if we decide to leave them as their own class now that they're static we can access them directly rather than having a member variable...
6) Code duplication, two copies of StringToInt()? Bad programmer. Remove one.
7) Unnecessary work. Why do you convert the network byte order address to a string and then the string to a network byte order dword and then the string to a host byte order dword?
8) nErrFlag, what's all that about then. Why not just use indentation within the if's rather than setting an error flag (which isnt really a "flag" anyway as it can have more than 2 values...) and switching on that (sometimes, but not always) and then ignoring it. Anyway, we could end up with something cleaner, like, perhaps, the code below (which hasnt seen a compiler).
bool LocalIPQuery::GetLocalIP()
{
bool ok = false;
WSAData wsaData;
if (WSAStartup(MAKEWORD(1, 1), &wsaData) == 0)
{
char ac[80];
if (gethostname(ac, sizeof(ac)) != SOCKET_ERROR)
{
m_csHostName = ac;
struct hostent *phe = gethostbyname(ac);
if (phe)
{
for (int i = 0; phe->h_addr_list[i] != 0; ++i)
{
m_csLocalAddress = inet_ntoa((in_addr)phe->h_addr_list[i]);
ok = true;
}
m_dwHostByteOrderLocalAddress = m_Converter.StringToHBO(csReturn);
m_dwNetworkByteOrderLocalAddress = m_Converter.StringToNBO(csReturn);
}
WSACleanup();
}
return ok;
}
9) If you must use hungarian warts use them consistently. In StringToInt() why is the loop index nLoop (crap name, but, well, whatever), yet Return and StringLength arent nReturn and nStringLength...
10) Err, return this->m_csLocalAddress; what's with that then?
So, overall, 2/10, must try harder.
Read these books on general coding and these ones on C++ then rewrite your article and we'll go through the process again.
Len Holgate
www.jetbyte.com
The right code, right now.
|
| Sign In·View Thread·PermaLink | 5.00/5 |
|
|
|
|
|
|
Len, did you see my comment?
I agree on your points, however I want to stress that even if code style fixes are applied the underlying approach is very wrong.
The task is to get local ip address. My main question is: what if machine has several IP addresses? No answer. Normally in this situation your return ip address of default interface (this is also what example in FAQ does). However the code in the article simply returns random - whichever is last in TCP connection table ! Do I need to continue?
Igor Proskuriakov
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
|
|
|
Is there a general rule of thumb to know when something should be a data member vs using a function call or two to convert a data member to another format?
Nate
You can kill the revolutionary, but you can't kill the Revolution. (RATM & TOOL)
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
It usually depends on whether speed or size is most important. If you only ever use format 2 on rare occassions and it takes up a lot of memory or takes a long time to perform the conversion, then it may be worth performing the conversion when you first need that format...
Len Holgate
www.jetbyte.com
The right code, right now.
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
Len Holgate wrote:
9) If you must use hungarian warts use them consistently. In StringToInt() why is the loop index nLoop (crap name, but, well, whatever), yet Return and StringLength arent nReturn and nStringLength...
Well, to be consistent in Hungarian notation, it should be "iLoop", since it's an (i)ndex, not a value/(n)umber... But it's not a big deal 
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
 | Useless 
Igor Proskuriakov | 4:38 20 Aug '02 |
|
|
Absolutely useless article!!!
As far as I understood from description, the goal is to get local IP address.
Normally everyone can do it easily using gethostbyname. See for example Winsock Programmer's FAQ
Example: Getting the Local IP Address
http://tangentsoft.net/wskfaq/examples/ipaddr.html
The main advantage of this approach that it is portable and does not require any Winsock specifics. By some unknown reasons author decides to use IP Helper. Even if IP Helper API provides well known way to get local address, which is GetAdaptersInfo, our Author decides to use GetTcpTable, which enumerates all exiting TCP connections. If you look into the code you see that author stupidly goes through every connection, gets local address, every time overwriting previous result and returns the last one. And this is bascially the core of that class!!!
I even can go into details and mention that this thing should not exist as a class as it does not care about any internal variable and all those member functions should become non member. See for example famous "How Non-Member Functions Improve Encapsulation" http://www.cuj.com/articles/2000/0002/0002c/0002c.htm.
Actually, the article maybe a joke but some readers might not understand it and even use this code!!!!
Igor Proskuriakov
|
| Sign In·View Thread·PermaLink | 5.00/5 |
|
|
|
|
|
|
Hey, take it easy, man.
His article is much better than yours.
As you by now have did none.
If you do not agree with the method, make sugestions and be less agressive, the article's author does not HAVE to work for your pleasure, after all.
Concussus surgo.
When struck I rise.
|
| Sign In·View Thread·PermaLink | 3.67/5 |
|
|
|
|
|
|
Daniel Turini wrote:
His article is much better than yours.
My what? My comment? why is it better?
Almost everything I wrote was true, why I cannot say the same about the article.
When you publish an article you always have to care about readers, you know wrong documentation is worse that the absense of documentation.
Igor Proskuriakov
|
| Sign In·View Thread·PermaLink | 5.00/5 |
|
|
|
|
|
|
Everyone is entitled to their opinion. I appreciate your feed back. My purpose with this class was to get the local machines IP address and to be able to take information from controls and convert it to a form that suits my needs. This class does that for me. I'm not a Winsocks guru and don't claim to be. I thank you for the following link as I was not aware of it.
Example: Getting the Local IP Address
http://tangentsoft.net/wskfaq/examples/ipaddr.html
I will definitely read it and continue with my learning process.
Nate
You can kill the revolutionary, but you can't kill the Revolution. (RATM & TOOL)
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
I'm on a firewall, and this only returns the 192.168.x.x number, which is my ip on my internal network. Any way to determine the ip that is sent out to websites when I'm viewing them?
Thanx,
Swinefeaster
Check out Aephid Photokeeper, the powerful digital
photo album solution at www.aephid.com.
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
I don't know of a way to do it programmatically. I'm not even close to being and expert on this, but I think your computer really only knows about what is on the internal network. I think the only device that has the outside IP address is your cable modem/DSL modem. There is a website (DSLreports) that will tell you your outside IP address, but that is not a programming solution. You might ask over at Farooque Khan's article.
Nate
You can kill the revolutionary, but you can't kill the Revolution. (RATM & TOOL)
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
You need an echo server of some sort to determine programmatically the ip exposed to the outside. You app would essentially ping the server, the information returned from the server would contain your address as it saw it.
|
| Sign In·View Thread·PermaLink | |
|
|
|
|
|
|
|
