Code review comment for lp://qastaging/~abreu-alexandre/webbrowser-app/container-print-help-when-no-appid

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

Thank you for your comments,

> BrowserApplication::isPrintHelpLaunch() doesn’t need to take an argument, it
> could be made non-static and read m_arguments.

I know, but I tend to prefer environment agnostic function when possible,
hence my approach there, I dont like "introspective" function that dont have to be,

> But there’s a cleaner approach I think: the check for -h or --help and the
> call to printUsage() could be done in the constructor
> (BrowserApplication::BrowserApplication()), and this would set a flag (maybe
> called "m_shouldExitEarly", or something like that) that the main function
> would test before calling initialize().

I am not sure if it is a cleaner approach, constructor work + state flag seem weird
to me,
What bothers you is the double calls to isPrintHelpLaunch?

> On a slightly related note, in WebappContainer::initialize() the call to
> earlyEnvironment() should go after the check for the appId.

+1, done

« Back to merge proposal