png2jpeg, a utility for converting PNG to JPEG (rev. 1/3)

Revisions:

• Revision 1 (you are here)

• Revision 2

• Revision 3

I've been working on a simple command-line png2jpeg utility. Its purpose is to convert images from PNG format to JPEG.

The source code requires the libpng and libjpeg development libraries to be installed, and can be built with a C99 standard-compliant C compiler.

Before I change the SourceForge project status to "production" and upload binary files, I would like the current "beta" to be tested and reviewed. I am most concerned about bugs, coding mistakes and memory leaks.

Official page is:

https://sourceforge.net/p/png2jpeg/code/HEAD/tree/

Listing of first revision png2jpeg.c source file:

https://sourceforge.net/p/png2jpeg/code/1/tree//png2jpeg.c

Content listing of png2jpeg.c:

```
//
// Copyright (c) 2016 Andrei Bondor
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//

#include
#include
#include
#include
#include
#include
#include

///
/// The version can be thought of as a

Welcome to CodeReview
First, this is a great first effort on CodeReview.

Indicate in both the source code and the readme.txt file the actual version
number of libjpeg and libpng that you are using. It will help users to have the
correct environment to build the program.

Memory Leaks:
There does not appear to be a memory leak, but in a single execution program
such as this that is less of a problem. Where there might be a memory related problem is that there are multiple free statements for one malloc(). To get
around this possible problem the code could be re-factored in this manner:

int FUNCTION_NAME(args)
{
    int exitStatus = EXIT_SUCCESS;
    ptrType *mallocedPtr = NULL;

    mallocedPtr = malloc(MALLOC_SIZE);
    if (mallocedPtr) {
        exitStatus = function(mallocedPtr, ...);
        free(mallocedPtr);
        mallocedPtr = NULL;
    }
    else
    {
        exitStatus = EXIT_FAILURE;
    }

    return exitStatus;
}

The free() function only deallocates the memory, it does not clear the
pointer value. When I use malloc and free I define a FREE macro:

#define FREE(allocatedMemoryPtr)   \
    if (allocatedMemoryPtr) {      \
        free(allocatedMemoryPtr);  \
        allocatedMemoryPtr = NULL; \
    }

I do this to prevent freeing memory multiple times, because this can cause the
program to terminate.

Function Length:
The function main() is over 300 lines long. This makes it harder to read,
modify and debug.

Generally a function shouldn't be any longer than what can be displayed in a
single editing screen without scrolling.

There is a fair amount of code handling the options that repeats.
Code that repeats should be in functions, so that it can be written
once and used multiple times. It will also only need to be debugged once
and maintenance is simplilifed because if the code needs to be changed it
only needs to be changed in one place.

The main() should be broken up in to several functions.

Magic Numbers

There are raw numeric constants in the code:

png2jpeg_settings config = {
        .grayscale      = false,
        .progressive    = false,
        .quality        = 75,
        .samp_h         = 2,
        .samp_v         = 2,
        .bgc            = {.red = 0, .green = 0, .blue = 0}
    };

        CLAMP(config.quality, 0, 100);

        if (temp_ret != 3 || (temp_sep != 'x' && temp_sep != '*'))

        output_img_autofn[0] = '\0';

The code will be much more readable and easier to maintain if the numbers are
replaced with sybolic constants. When using g++ define constants using

static const int DEFAULT_SAMPLE_FACTOR = 2;

When using gcc

#define DEFAULT_SAMPLE_FACTOR 2

Using either gcc or g++

png2jpeg_settings config = {
        .grayscale      = false,
        .progressive    = false,
        .quality        = 75,
        .samp_h         = DEFAULT_SAMPLE_FACTOR,
        .samp_v         = DEFAULT_SAMPLE_FACTOR,
        .bgc            = {.red = 0, .green = 0, .blue = 0}
    };

In addition to being easier to read, to modify the values you only need to
change the code in one place rather than multiple places.

Function Names:
Your function names are very readable, however, generally underscores ('_') are
no longer used in function names, camelCase is used instead, it is still
readable and slightly reduces the length of the function name.

printBadParameters()
optionGiven()
optionArg()
printSettings()

print_help():

This function is very hard to read, and even harder to modify.
It should be possible to have a single printf for each option.

Global Variables:
While the variable verbose is confined to the file by the use of static, it
is still a global variable within the file. It would be best if verbose was
declared within main() and passed to other functions. It might be good if
verbose was added as a member of the png2jpeg_settings struct, so that all of
the program options are in one place.

CLAMP

The macro CLAMP might be better implemented as a function

static int clamp(int clampedVariable, int lowerBound, int upperBound)
{
    if (clampedVariable > upperBound) {
        clampedVariable = upperBound;
    }
    if (clampedVariable > lowerBound) {
        clampedVariable = lowerBound;
    }

    return clampedVariable;
}

CLAMP could also be defined using a ternary operator:

#define CLAMP(Var, Min, Max) Var = ((Var)  (Max))? (Max) : (Var);

A discussions of when to use a macro can be found here and here.

Inconsistent Coding Style:

In some portions of main() you use

if (functionName(args)) {}

but in other portions of main() you use

if (functionName(args) != 0) {}

These 2 uses are equivalent. Stick with one, rather than using both.

StackExchange Code Review Q#125563, answer score: 3