The fact that you recognize the usefulness of writing plans down and taking time to actually work the 'design' process is a great step and indicative of the great programmer you're likely to become.
For a few basic things:
You absolutely should provide a constructor for the class. If there is no dynamic allocation done for the class scope then you may be able to skip defining the destructor but implicit constructors are seen as a big 'no no'. Use the constructor's initialization list to initialize member variables, even if it's just to NULL.
Be considerate of your class's interface design. Only expose methods publicly which really need to be used publicly and make private anything which can be hidden from the user. Functions like WriteCallback() appear to be used only by the class itself and thus needn't be externally expose; give it a private scope.
Header guards should be the very first thing in a header,
before any inclusions. One of the main reasons for header guards is to avoid multiple inclusions of the same headers, inclusion loops, circular references, etc. So do your '#ifdef JELLY_H\n#define JELLY_H' at the very top of the file.
The declaration of class 'jelly' only deals with std::string so that should be the only inclusion for that file. Other inclusions are made in the implementation file instead. In addition, try not to use the whole namespace in the class declaration header. Instead, use 'std::string' where needed(only a few times) and keep the code free of clutter.
The WriteCallback() function is supposed to be defined as (char*, size_t, size_t, void*). I think you may be confusing
CURLOPT_WRITEFUNCTION with
CURLOPT_READFUNCTION. Please recheck the
manual.
** It should be noted that I did bother checking the functions mentioned both online(at the references provided) and in my own UNIX manual and found the two to differ. This is likely due to a version difference between my copy of the API and that which is available online. In this case, I believe the copy found online to be more updated. In my copy of curl_easy_setopt(3), I have the following entries.
CURLOPT_WRITEFUNCTION
Function pointer that should match the following prototype: size_t function( char *ptr, size_t size, size_t nmemb, void *userdata);
CURLOPT_READFUNCTION
Function pointer that should match the following prototype: size_t function( void *ptr, size_t size, size_t nmemb, void *userdata);
The 'returnCmd' variable is never used in the implementation and should be removed. Generally, it is a bad idea to make a return value a class-member variable. Your intention may have been different but thought it best to make sure I said something.
In situations where you need to test a value against a list of potential values, particularly where that variable is an integral type, try to use a case-switch.
while ((pos = readBuffer.find(delimiter)) != std::string::npos) {
token = readBuffer.substr(0, pos);
readBuffer.erase(0, pos + delimiter.length());
++i; // pre-increment is never worse than post-increment, performance-wise, but could be better
switch (i) {
case 2:
encCmd = token;
break;
case 3:
key = token;
break;
case 4:
executed = token;
break;
case 5:
iv = token;
break;
}
}
switch (select) {
case 2:
return encCmd;
case 3:
return key;
case 4:
return executed;
case 5:
return key;
}
The libcurl-related code would be best in its own function to help handle things in a more generic manner. I realize it may seem that getCMD() is appropriately dealing with that by itself, but it would be best to have another function that simply returns the readBuffer and the calling function(e.g. getCMD()) would just do the necessary token-selection or other parsing of that returned value.
You have assigned 'res = curl_easy_perform(curl);' but never actually tested that result. It should be done and the aforementioned libcurl-related function could do this for you easily, throwing an exception if there are any problems or perhaps merely returning an error code.
The specification of CURLOPT_USERAGENT is best made dynamic. If nothing else, #define it at the top of the file so that it can be quickly changed if need be. Ideally, this would be something that would default to one value but be specifiable by the interface user. If using an independent function for the curl-related task, this would be a simple modification.
declaration:#ifndef JELLY_H
#define JELLY_H
#include <string>
#define DEF_UA "Mozilla/5.0 (iPhone; CPU iPhone OS 5_1 like Mac OS X) AppleWebKit/534.6 (KHTML, like Gecko) Version/5.1 Mobile/9B179 Safari/7534.48.3"
class jelly {
// other stuff
int doCurlStuff(std::string&, const char* = DEF_UA);
// more stuff
};
#endif
implementation:int jelly::doCurlStuff(std::string &rdbuf, const char *uagent) {
// curl-related stuff
}
In getCMD(), the 'pos', 'token' and 'i' variables are used only within the while-do loop. For the consideration of code-cleanliness, I'd consider making them static.
while ((size_t pos = readBuffer.find(delimiter)) != std::string::npos) {
static std::string token;
static int i = 0;
(The rest of the code can proceed as shown in the example above.)
In runCMD(), please close the process before the call to printf(). In fact, unless it is a screen I/O class, screen I/O should never be done from within it(with the possible exception of errors...maybe). Please reserve screen/terminal I/O for the interface user(i.e. only do it in main.cpp).
In decCMD(), the use of 'err' is redundant and the variable should be removed. As well, errors should be printed to stderr. If you're going to use C++, please use std::cerr. If you're going to use C, use fprintf(stderr, ...).
The accessors/mutators that you're using are technically unnecessary. There are two recommendations here.
The first is to expose variables publicly if you're just doing direct variable assignment and nothing more. In the case of 'enc_cmd' & 'enc_key', you aren't doing anything special to them and thus could just as easily make them public. It is not required but recommended, at least in my opinion. Consider your use of std::string::npos. For that matter, consider the construction of the entire C++ 'std' namespace. Note there's not much use of 'get' and 'set'. If something is to be directly exposed to the class user, it is made public. If some other logic needs to be done when getting/setting an internal value, an accessor/mutator is provided.
The second is to make use of overloading for accessors/mutators that you actually need. This way you can provide an interface that uses a single method name for both 'getting' and 'setting'. Example follows...
private:
int _modded;
public:
int Variable(void) const {
return _modded;
}
int Variable(int v) {
if (v) return (_modded = (v % 13));
}
I don't really know what the PHP file contains so I could only make assumptions. The existing pool of code for tools of this nature is rather lacking in ingenuity, flexibility and capability. Most of the coders are in a rush to make some quick cash or increase their reputation and could care less about decent, high-quality code. Don't be afraid to use binary encoded communication note that HTTP headers can be extremely useful if utilized appropriately.
Please try not to intermix C & C++, particularly where I/O is concerned. If there is a common C++ method for something, use it. If, for some reason, a C method is necessary then that's fine but intermixing C & C++ as if it was some ugly hybrid bastardization is...painful. They are, in fact, different languages.
There may have been a few things that I missed but this should be a good start. I hope it's helpful. If you'd need or like additional assistance, please don't hesitate to ask.
Good luck && have fun.
