patterncppMinor
Small command-line helper tool
Viewed 0 times
linehelpersmallcommandtool
Problem
I've coded a small command line helper tool for this library I'm working on. The library provides tools for the use of Virtual Texturing on iOS devices (mainly games).
This little command line helper is still pretty much a prototype that I coded as quick as I could, for testing a new file format. Some feedback would be appreciated before I expand it further:
```
// Virtual Texturing Library:
#include "vt_tool_image.hpp"
#include "vt_tool_pagefile_builder.hpp"
#include "vt_tool_platform_utils.hpp"
// Standard library:
#include
#include
#include
#include
#include
namespace {
// ======================================================
// Local data:
// ======================================================
vt::tool::PageFileBuilderOptions cmdLineOpts;
std::string inputFile, outputFile;
// ======================================================
// printHelpAndExit()
// ======================================================
void printHelpAndExit()
{
std::printf("\n"
"Usage:\n"
"$ vtmake [--flags=]\n"
"\n"
"Flags accepted:\n"
"--help : prints help text with list of commands.\n"
"--filter : (str) type of mipmapping filter: box, tri, quad, cubic, bspline, mitchell, lanczos, sinc, kaiser.\n"
"--page_size : (int) total page size in pixels, including border.\n"
"--content_size : (int) size in pixels of page content, not including border.\n"
"--border_size : (int) size in pixels of the page border.\n"
"--max_levels : (int) max mipmap levels to generate.\n"
"--flip_v_src : (bool) flip the source image vertically.\n"
"--flip_v_tiles : (bool) flip each individual tile/page vertically.\n"
"--stop_on_1_mip : (bool) stop subdividing when mip 0 is reached.\n"
"--add_debug_info : (bool) print debug text to each page.\n"
"--dump_images : (bool) dump each page as an image file (TGA format).\n"
"--verbose : (bool) print stuff to STDOUT while running.\n"
This little command line helper is still pretty much a prototype that I coded as quick as I could, for testing a new file format. Some feedback would be appreciated before I expand it further:
```
// Virtual Texturing Library:
#include "vt_tool_image.hpp"
#include "vt_tool_pagefile_builder.hpp"
#include "vt_tool_platform_utils.hpp"
// Standard library:
#include
#include
#include
#include
#include
namespace {
// ======================================================
// Local data:
// ======================================================
vt::tool::PageFileBuilderOptions cmdLineOpts;
std::string inputFile, outputFile;
// ======================================================
// printHelpAndExit()
// ======================================================
void printHelpAndExit()
{
std::printf("\n"
"Usage:\n"
"$ vtmake [--flags=]\n"
"\n"
"Flags accepted:\n"
"--help : prints help text with list of commands.\n"
"--filter : (str) type of mipmapping filter: box, tri, quad, cubic, bspline, mitchell, lanczos, sinc, kaiser.\n"
"--page_size : (int) total page size in pixels, including border.\n"
"--content_size : (int) size in pixels of page content, not including border.\n"
"--border_size : (int) size in pixels of the page border.\n"
"--max_levels : (int) max mipmap levels to generate.\n"
"--flip_v_src : (bool) flip the source image vertically.\n"
"--flip_v_tiles : (bool) flip each individual tile/page vertically.\n"
"--stop_on_1_mip : (bool) stop subdividing when mip 0 is reached.\n"
"--add_debug_info : (bool) print debug text to each page.\n"
"--dump_images : (bool) dump each page as an image file (TGA format).\n"
"--verbose : (bool) print stuff to STDOUT while running.\n"
Solution
-
Do not hardcode
-
Each
I would also recommend to restructure code to let
-
The loop in
-
The overall command line structure is quite unconventional. Typically positional parameters follow options. Also it is very useful to make filenames optional as well: such tool can be then be used in a pipeline.
-
I don't see the need for
-
Finally, why not just use
-
A side note:
doesn't seem right. The
Do not hardcode
vtmake in printHelpAndExit Print argv[0] instead.-
Each
parseWhatever function has its own copy of code finding the =. This must be factored out into a function; even better, use strchr.I would also recommend to restructure code to let
parseCmdLine find the =. -
The loop in
parseCmdLine contains too much code. I would rather have a map of options to parsing functions. Similarly, I'd have a map of filter names to vt::tool::FilterType values.-
The overall command line structure is quite unconventional. Typically positional parameters follow options. Also it is very useful to make filenames optional as well: such tool can be then be used in a pipeline.
-
I don't see the need for
cmdLineOpts, inputFile, outputFile to exist in a namespace scope. I recommend to define them in main and pass them to interested parties as parameters.-
Finally, why not just use
getopt?-
A side note:
vt::tool::PageFileBuilder pageFileBuilder(inputFile, outputFile, cmdLineOpts);
pageFileBuilder.generatePageFile();doesn't seem right. The
PageFileBuilder looks like an implementation detail of generatePageFile, and should not be exposed to client.Code Snippets
vt::tool::PageFileBuilder pageFileBuilder(inputFile, outputFile, cmdLineOpts);
pageFileBuilder.generatePageFile();Context
StackExchange Code Review Q#70620, answer score: 3
Revisions (0)
No revisions yet.