A good code review starts by having the requester indicate clearly what he wants to gain from the review. What do you want to learn from this?Originally Posted by Richardcavell
Around here, I've participated in code reviews ranging from informal cubicle conversations to brief reviews of isolated code sections to marathon week long exhaustive review of a new 15,000 line module.
Code://try //{ if (a) do { f( b); } while(1); else do { f(!b); } while(1); //}
What I want is to :
Find bugs
Find portability issues
Find ways of reusing code paths
Enhance readability of the code
in that order. Performance is not really an issue.
Last edited by Richardcavell; 03-09-2011 at 03:48 PM. Reason: redundancy
Testing is a far more effective way of finding bugs than code review.
Let us know if you post your codebase up somewhere. I'd be hesitant to use SourceForge, because they require you to declare which license you're going to use, and the choice of license is not a decision to skim over quickly.
Code://try //{ if (a) do { f( b); } while(1); else do { f(!b); } while(1); //}
I've uploaded it here:
RichardcavellBot | Download RichardcavellBot software for free at SourceForge.net
I did spend a day thinking about licenses, but I'm happy with what I've chosen.
Well... I had a bit of a look at perhaps 40 or 50 of the files...
Right off the top your code is very inefficient...
Tip 1 : when one function depends on the completion of a function before it, you don't need to call the function, test the result, print an error message then exit...
Instead you can do this... The only way to return success is if all functions succeed... where anything that fails the chain is broken and the function will return failure.Code:res = function1() if (res == -1) return -1; res = function 2() if (res == -1) return -1;
Code:if ( function1() ) if ( function2() ) if ( function3() ) return success; return failure;
tip 2 : Also there were many times where you used case statements like this...
When you could simply have usedCode:res = function1(); switch (res) { case -1 : return -1; case -2 : return -2; case -3 : return -3; }
I also spotted about half a dozen places where you used single equals signs in if statements and other minor syntax errors.Code:return function1();
to give you an example...
You wrote...
This can be reduced to...Code:int rovepageeditnecessary ( void ) /* 1 = edit is necessary. 0 = edit is not necessary. */ { if ( !uiwikitextbuffervalid ) { error ( __FILE__ , __LINE__ , "Wikitext buffer does not contain valid wikitext." ) ; return -1 ; } int res ; res = detectrovevandalism ( pwikitextbuffer ) ; switch ( res ) { case -1 : return -1 ; case 0 : return 0 ; case 1 : return 1 ; default : break ; } error ( __FILE__ , __LINE__ , "Unreachable code." ) ; return -1 ; }
tip 3 : I noticed that every one of your functions is defined as int FunctionName (void) ... which tells me you must have bags and bags of global variables floating around. There are so many reasons not to use global variables unless it is absolutely necessary and even then, only within a single source page... name collisions, things going out of scope, all kinds of weirdness can happen. You seriously need to learn how to pass values into functions.... From the example above, I would have passed in the pointer to the wikitestbuffer instead of making it global.Code:int rovepageeditnecessary ( void ) /* 1 = edit is necessary. 0 = edit is not necessary. */ { if ( uiwikitextbuffervalid ) return detectrovevandalism ( pwikitextbuffer ) ; return -1 ; }
Stuff like this just doesn't cut it...
IOW... from a brief survey, I'd say there's enough room for improvement that at least 50% of your functions could be (should be?) rewritten.Code:#include "Buffers.h" char* pheaderbuffer = 0 ; char* pbodybuffer = 0 ; char* pwikitextbuffer = 0 ; char* peditbuffer = 0 ; char* phtmlrenderbuffer = 0 ; char* ppastrevidbuffer = 0 ; unsigned int uiwikitextbuffervalid = 0 ; unsigned int uieditbuffervalid = 0 ; unsigned int uihtmlrenderbuffervalid = 0 ; unsigned int uipastrevidbuffervalid = 0 ; char szeditsummary [ editsummarymax ] ; char szedittoken [ edittokenmax ] ; char szlogintoken [ logintokenmax ] ; char sztimestamp [ timestampmax ] ; char szauthor [ authormax ] ; char szpastauthor [ authormax ] ; char szpastrevid [ authormax ] ; unsigned int uieditsummaryvalid = 0 ; unsigned int uiedittokenexists = 0 ; unsigned int uilogintokenexists = 0 ; unsigned int uitimestampvalid = 0 ; unsigned int uiauthorvalid = 0 ; unsigned int uipastauthorvalid = 0 ; unsigned int uipastrevidvalid = 0 ; ///////////////////////////////////////////////////////// // buffers .h // #ifndef BUFFERSHHASBEENINCLUDED #define BUFFERSHHASBEENINCLUDED 1 #include "Variables.h" /* Get the #defines. */ extern char* pheaderbuffer ; /* This is where the raw HTML header goes. */ extern char* pbodybuffer ; /* This is where the raw HTML body goes. */ extern char* pwikitextbuffer ; /* The original wikitext goes here. */ extern char* peditbuffer ; /* This is the one our edit goes into. */ extern char* phtmlrenderbuffer ; /* The original HTML render goes here. */ extern char* ppastrevidbuffer ; /* The previous revid goes here. */ extern unsigned int uiwikitextbuffervalid ; extern unsigned int uieditbuffervalid ; extern unsigned int uihtmlrenderbuffervalid ; extern unsigned int uipastrevidbuffervalid ; extern char szeditsummary [ editsummarymax ] ; extern char szedittoken [ edittokenmax ] ; extern char szlogintoken [ logintokenmax ] ; extern char sztimestamp [ timestampmax ] ; extern char szauthor [ authormax ] ; extern char szpastauthor [ authormax ] ; extern char szpastrevid [ revidmax ] ; extern unsigned int uieditsummaryvalid ; extern unsigned int uiedittokenexists ; extern unsigned int uilogintokenexists ; extern unsigned int uitimestampvalid ; extern unsigned int uiauthorvalid ; extern unsigned int uipastauthorvalid ; extern unsigned int uipastrevidvalid ; #endif //////////////////////////////////////////////////////////////////////// // variables.h // #ifndef VARIABLESHHASBEENINCLUDED #define VARIABLESHHASBEENINCLUDED 1 #include <stddef.h> /* To get size_t. */ #define stringmax 200 #define logintokenmax 100 #define edittokenmax 100 #define urlmax 200 #define editsummarymax 200 /* This is Wikipedia's limit. */ #define timestampmax 100 #define authormax 100 #define revidmax 100 extern const size_t buffersize ; extern unsigned int uiverbose ; extern unsigned int uiprogressmeter ; extern unsigned int uiimprovementscanloop ; extern unsigned int uisilentmode ; extern unsigned int uistatusupdate ; extern unsigned int uipersevere ; extern unsigned int uidumpbuffers ; extern unsigned int uisimulate ; extern char szmaxlag [] ; extern const unsigned int uinumberofattempts ; extern unsigned int uisecondsbetweenimprovements ; extern unsigned int uisecondsbetweenimprovementscans ; extern unsigned int uisecondsbetweenfailedhttprequests ; extern unsigned int uiextrasecondsbetweenthrottledloginrequests ; extern const char szstartupmessage [] ; extern const char szshutdownmessage [] ; extern const char szusagemessage [] ; extern const char szunrecognizedparameter [] ; extern const char szversionmessage [] ; extern const char szuseragent [ stringmax ] ; /* Needs to have a size because the curl function is a macro. */ extern const char szloginurl [] ; extern const char szlogouturl [] ; extern const char szedittokenqueryurl [] ; extern const char szwikitexturlprefix [] ; extern const char szhtmlrenderurlprefix [] ; extern const char szqueryurlprefix [] ; extern const char szediturl [] ; extern const char szpastauthorurlprefix [] ; extern const char szpastauthorurlmiddle [] ; extern const char szpastrevidurlprefix [] ; extern const char szusertalkprefix [] ; extern const char sztestpagetitle [] ; extern const char szstatuspagetitle [] ; extern const char szrovepagetitle [] ; extern const char szshutoffpagetitle [] ; extern const char szstatuspageeditsummary [] ; extern const char szstatusupdate [] ; extern const char szeditsummaryprefix [] ; extern const char szuserwarnurlprefix [] ; extern const char szrovewarntext [] ; extern const char szrovewarneditsummary [] ; extern const char szroverestoreeditsummary [] ; extern const char szheaderbufferfilename [] ; extern const char szbodybufferfilename [] ; extern const unsigned int uiminedittokenlength ; extern const unsigned int uimaxedittokenlength ; extern const unsigned int uiminlogintokenlength ; extern const unsigned int uimaxlogintokenlength ; extern const unsigned int uimintimestamplength ; extern const unsigned int uimaxtimestamplength ; extern const unsigned int uiminauthorlength ; extern const unsigned int uimaxauthorlength ; extern const unsigned int uiminrevidlength ; extern const unsigned int uimaxrevidlength ; #endif
Last edited by CommonTater; 03-09-2011 at 06:59 PM.
Wow, there really are a bajillion files.
I only sampled randomly, but from what I see, the ONLY action you ever take when something returns an error, is to propagate the error upward. Is there ANYWHERE in the code you actually try to recover from an error?
If you never actually recover, then ALL of this complexity can be eliminated. Whenever an errors happens, just call exit(1).
While I very much respect your dedication to error checking, for all of this effort it doesn't do anything helpful, and professional software is not written in this manner.
I'll try to review more later.
EDIT: By the way, despite glaring problems, I really, really do admire your dedication to a project idea. Hell, even I rarely get this far in my own home projects.
Last edited by brewbuck; 03-09-2011 at 06:47 PM.
Code://try //{ if (a) do { f( b); } while(1); else do { f(!b); } while(1); //}
And never in 12 days...
Thanks for all of your ideas.
Some of the redundancy (case statements etc) is for future expandability. The code always recovers from recoverable errors and even fatal errors are not fatal if the command line option 'persevere' is invoked. Fatal errors only occur if the Internet drops out or if I've programmed something incorrectly. Otherwise the bot runs for days without an error.
I'll work on reducing the global variables, redundant result checks, etc.
Richard
I'm having real trouble getting rid of these global variables. The reason why they're global is because they're accessed from functions that are not immediately related to each other.
Is getting rid of globals really a priority? The command line options alone set about 14 variables. Should I parse the command line options within main (), make the variables local to main() and then send the relevant ones through function calls to their destinations?
Last edited by Richardcavell; 03-09-2011 at 07:42 PM.
Right now they're nothing but sources of error that affect performance for no good reason whatsoever. You don't leave code at loose "just in case", you add it when you need it.
Yes, it will... but not because of your overly zealous error checking.The code always recovers from recoverable errors and even fatal errors are not fatal if the command line option 'persevere' is invoked. Fatal errors only occur if the Internet drops out or if I've programmed something incorrectly. Otherwise the bot runs for days without an error.
Absolutely. In my experience the biggest sources of almost untraceable errors have always been related to global variables.I'm having real trouble getting rid of these global variables. The reason why they're global is because they're accessed from functions that are not immediately related to each other.
Is getting rid of globals really a priority?
Take a look at my remote media server/client ... here in all three programs only the settings are global.
That would be a start.The command line options alone set about 14 variables. Should I parse the command line options within main (), make the variables local to main() and then send the relevant ones through function calls to their destinations?
Okay,
Let's see if I'm on the right track. I took the first function called by main(), which parses the command line, and rolled it into main() itself. This means that all configuration variables can become local variables of main() instead of globals. I've also shaved down the line count considerably by eliminating redundancy. This eliminates two source files (Parsecommandline.c and Parsecommandline.h) and a total of about 150 lines of source.
Am I on the right track?
Richard
Code:#include <stdlib.h> #include <string.h> #include <curl/curl.h> #include "Startupandshutdownbot.h" #include "Improvewikipedia.h" #include "Safeprint.h" #include "Variables.h" int main ( int argc , char* argv [] ) { unsigned int uiversiondisplay = 0 ; /* If set to 1, we will exit without improving Wikipedia. */ unsigned int uihelpdisplay = 0 ; /* If set to 1, we will exit without improving Wikipedia. */ unsigned int uibadparameter = 0 ; /* If set to 1, we will exit without improving Wikipedia and return failure. */ for ( int arg = 1 ; arg < argc ; arg ++ ) /* Parse command line options. */ { char *pparameter = argv [ arg ] ; if ( strcmp ( pparameter , "highmaxlag" ) == 0 ) /* highmaxlag */ strcpy ( szmaxlag , "20" ) ; else if ( strcmp ( pparameter , "nodump" ) == 0 ) /* nodump */ uidumpbuffers = 0 ; else if ( strcmp ( pparameter , "nolog" ) == 0 ) /* nolog */ uinolog = 1 ; else if ( strcmp ( pparameter , "noloop" ) == 0 ) /* noloop */ uiimprovementscanloop = 0 ; else if ( strcmp ( pparameter , "noshutoff" ) == 0 ) /* noshutoff */ uinoshutoff = 1 ; else if ( strcmp ( pparameter , "nostatus" ) == 0 ) /* nostatus */ uistatusupdate = 0 ; else if ( strcmp ( pparameter , "persevere" ) == 0 ) /* persevere */ uipersevere = 1 ; else if ( strcmp ( pparameter , "progressmeter" ) == 0 ) /* progressmeter */ uiprogressmeter = 1 ; else if ( strcmp ( pparameter , "quick" ) == 0 ) /* quick */ { uisecondsbetweenimprovements = 2 ; uisecondsbetweenimprovementscans = 2 ; uisecondsbetweenfailedhttprequests = 5 ; uiextrasecondsbetweenthrottledloginrequests = 120 ; } else if ( strcmp ( pparameter , "silent" ) == 0 ) /* silent */ uisilentmode = 1 ; else if ( strcmp ( pparameter , "simulate" ) == 0 ) /* simulate */ uisimulate = 1 ; else if ( strcmp ( pparameter , "verbose" ) == 0 ) /* verbose */ uiverbose = 1 ; else if ( ( strcmp ( pparameter , "v" ) == 0 ) /* version (or one of 5 synonyms) */ || ( strcmp ( pparameter , "version" ) == 0 ) || ( strcmp ( pparameter , "-v" ) == 0 ) || ( strcmp ( pparameter , "-version" ) == 0 ) || ( strcmp ( pparameter , "--v" ) == 0 ) || ( strcmp ( pparameter , "--version" ) == 0 ) ) uiversiondisplay = 1 ; else if ( ( strcmp ( pparameter , "h" ) == 0 ) /* help (or one of 5 synonyms) */ || ( strcmp ( pparameter , "help" ) == 0 ) || ( strcmp ( pparameter , "-h" ) == 0 ) || ( strcmp ( pparameter , "-help" ) == 0 ) || ( strcmp ( pparameter , "--h" ) == 0 ) || ( strcmp ( pparameter , "--help" ) == 0 ) ) uihelpdisplay = 1 ; else { if ( safeprintf ( __FILE__ , __LINE__ , "Unrecognized command line option : %s\n" , pparameter ) < 0 ) return EXIT_FAILURE ; uibadparameter = 1 ; } } if ( uibadparameter ) { uisilentmode = 0 ; safeprint ( __FILE__ , __LINE__ , szusagemessage ) ; return EXIT_FAILURE ; } if ( uiversiondisplay ) { uisilentmode = 0 ; if ( safeprint ( __FILE__ , __LINE__ , szversionmessage ) < 0 ) return EXIT_FAILURE ; if ( safeprintf ( __FILE__ , __LINE__ , "Libcurl version : %s\n" , curl_version() ) < 0 ) return EXIT_FAILURE ; } if ( uihelpdisplay ) { uisilentmode = 0 ; if ( safeprint ( __FILE__ , __LINE__ , szusagemessage ) < 0 ) return EXIT_FAILURE ; } if ( ( uiversiondisplay ) || ( uihelpdisplay ) ) return EXIT_SUCCESS ; /* Exit without improving Wikipedia. */ int res = startupbot () ; /* Startup sequence. */ if ( res < 0 ) { shutdownbot () ; return EXIT_FAILURE ; } do { res = improvewikipedia () ; /* The main stuff. */ } while ( ( res == -1 ) && ( uipersevere ) ) ; /* If we're persevering, ignore fatal return codes. */ if ( res < 0 ) { shutdownbot () ; return EXIT_FAILURE ; } res = shutdownbot (); /* Shutdown sequence. */ if ( res < 0 ) return EXIT_FAILURE ; return EXIT_SUCCESS ; }
safeprintf() and safeprint() seem like overkill. I mean, really, is it your problem if printf() doesn't work? I don't think I've ever seen anybody check the return value of printf(). It's just one of those things that never happens because it's way too much overhead.
But if you want to keep doing that, don't you find it tedious to always write __LINE__ and __FILE__? You should use a macro to wrap the function instead:
Then, of course, just rename safeprintf() to safeprintf_() in Safeprint.c, and you can use safeprintf("Foo").Code:/* header file */ int safeprintf_(const char *, unsigned, const char *, ...); #define safeprintf(...) safeprintf_(__FILE__, __LINE__, __VA_ARGS__)
WOW... you have got to be the most unbelievably verbose programmer I've ever seen...
One example...
You have a variable for uiversion display... if it's 1 you exit...
Make that a function call... show the version and exit. No variables required.
The same for help... call a function to show help and exit. No variable required.
Creating all these flag variables that just end up triggining an exit is silly... whatever can be a function called directly from the command line parser should be.
Like this...
Really it doesn't need to be any more complex than that. Show help... same deal, a couple of quicky statements and an exit. No need for global this or flag that... just do it.Code:#define VERSION "WIKIBOT V.0.0.5" void ShowVersion(void) { printf("%s\n\n", VERSION); exit(0); } // cmdl processing else if ( strcmp ( argv[arg] , 'v' ) == 0 ) ShowVersion();
If I may make a general comment without offending you beyond constructive criticizm I would offer a thought my father used to hand me all the time: "Just enough knowledge to be dangerous" ... meaning that one is smart enough to handle the tools but really doesn't know what they are doing.
safeprint also logs the screen's output to a disk file ("Log.txt"). And just you wait till I get my hands on the ncurses library. It's much more than just printf.
Performance isn't really an issue with this program. I want it to be bug-free, robust, portable code. And thanks for the macro idea.
And yet, magically, it's none of those things.
I want my code to display all unrecognized parameters, display help if requested to do so, and display version info if requested to do so. This means that the whole command line must be parsed each time and I need the flags to make sure nothing is missed. It's the behaviour that I want.
