7
\$\begingroup\$

I am a beginner. I wrote a C program that calculates the first n powers of x and I don't even know if this is legit C code or strictly C++ code. Please criticize it and help me improve it.

#include <stdlib.h> //to use malloc and delete
#include <stdio.h> //to use printf

#define uint64 unsigned long long int //for simplicity

/* this program calculates the first n powers of x*/

uint64* Pow (int x, unsigned char n)
{
    uint64* retVal = (uint64*)malloc(sizeof(uint64));
    *retVal = 1;

    for (unsigned char i = 1; i <= n; i++)
    {
        *retVal *= x;
    }

    return retVal;
}

void calculateFirstNPowers(int x, unsigned char n)
{
    uint64 sum = 0;
    uint64* Pow_retValPtr;

    for (unsigned char i = 1; i <= n; i++)
    {
        Pow_retValPtr = ::Pow(x, i);

        printf("%d. Power of %d: ", i, x);
        printf("%d\n", *Pow_retValPtr);

        sum += *Pow_retValPtr;

        free(Pow_retValPtr);
    }

    printf("\nsum of first %d powers of %d: %d", n, x, sum);
}

int main()
{
    calculateFirstNPowers(2, 7);
    return 0;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ I'm curious to know what compiler you are using and what command-line flags you are invoking it with to build your program. \$\endgroup\$ Commented Jul 19, 2022 at 20:51
  • \$\begingroup\$ @200_success MSVC (cl command on CMD) and I don't use any flags. \$\endgroup\$ Commented Jul 20, 2022 at 12:46

3 Answers 3

Reset to default
6
\$\begingroup\$

Welcome to Code Review!

C or C++

Technically, this could be compiled as both C and C++, were it not for line 28:

Pow_retValPtr = ::Pow(x, i);

The scope resolution operator :: is only defined in C++, compiling this as C doesn't work.

Since I don't know too much about good C++, I'll treat this as if it was a C program as it reads more like C to me. Maybe someone more knowledgeable can add something about C++ in another answer.

uint64

Use typedef instead of #define:

typedef uint64 unsigned long long int;

The name also might cause confusion with the typedef uint64_t found in inttypes.h. Consider using this header directly instead of defining your own type.

Pow()

Why does this function not return the value directly? This way, there wouldn't be the need to allocate memory inside the function and then remember to free it outside the function. If you forget the latter part, some memory will be leaking every time the function is called, which is undesirable. Here's how the function would look:

uint64 Pow (int x, unsigned char n)
{
    uint64 retVal = 1;

    for (unsigned char i = 1; i <= n; i++)
    {
        retVal *= x;
    }

    return retVal;
}

You could also pass a pointer to a result variable as an argument to the function, but that makes the function a bit weird to use. How to do this is left as an exercise for the reader.

Printing

This shouldn't compile without any warnings. Both sum and Pow_retValPtr are/refer to a value of the type uint64/unsigned long long int, but the format %d is used for signed integers. The correct format to use is %llu.

The last printf doesn't end with a line break which looks ugly when the program is run from a terminal:

mindoverflow@pc:~/$ ./a.out
1. Power of 2: 2
...
5. Power of 2: 32

sum of first 5 powers of 2: 62mindoverflow@pc:~/$ 
mindoverflow@pc:~/$

Please add one!

Variable naming

x and n could be more descriptive. I'd suggest something like this:

uint64 Pow (int base, unsigned char power) ...

void calculateFirstNPowers(int base, unsigned char n)...

In the second case, the N in the function name describes the argument, so I didn't change it.

Pow_retValPtr could also just be renamed to power, as this is descriptive enough in the context IMO, especially, if you change Pow() as described above.

There also is a joke about naming a variable powerptr somewhere here ... :)

\$\endgroup\$
1
  • \$\begingroup\$ this is so extremely helpful. thank you. Also I made Pow() return a pointer because I didn't want to call the function Pow() 2 times inside calculateFirstNPowers() and I didn't want to copy the value it returns to a new variable. So I decided to use pointers. Thank you because I didn't know I have to use "%llu" and there is no :: in C. I learned so much useful stuff from your answer. \$\endgroup\$ Commented Jul 20, 2022 at 12:50
4
\$\begingroup\$

There are a few things that is worth pointing out from strictly the C++ point of view.

The code you have written, while still being valid C++, seems structured kind of in a C-fashioned way; there are a lot of C++ specific features that may be used to improve it.

C Headers

In C++ including stdio.h should be avoided. The C++ standard library provides the iostream header for IO operations. For example, the two following snippets are equivalent:

// C-fashion
#include <stdio.h>

void print_sum(int a, int b) {
    printf("%i\n", a + b);
}

// C++ fashion
#include <iostream>

void print_sum(int a, int b) {
    std::cout << a + b << std::endl;
}

I'm not sure whether the inclusion of C headers is still possible in C++, if it's just a compiler extension, or if it's just intended for backwards compatibility.

Allocation

Avoid using C dynamic allocation in C++ (malloc, calloc... etc stdlib.h functions). C++ introduces the new keyword. Refer to this SO question.

Headers

In the rare occasions where you really need a C header included. C++ has its own bindings for that.

#include <cstdio>  // C's stdio.h
#include <cstdlib> // C's stdlib.h

Take a look at this C++ reference for .

Naming convention

This is just a suggestion, you are free to use whatever naming convention you feel comfortable with as long as your code is not supposed to be part of a pre-existing project. Consistency is the first thing. I would rather use lowercase_with_underscores names for local variables and functions. I'd suggest you to take a look at Google C++ Style Format.

Apart from this there's the define thing of mindoverflow's answer

\$\endgroup\$
4
  • 1
    \$\begingroup\$ but I want it to be C code \$\endgroup\$ Commented Jul 20, 2022 at 13:03
  • 1
    \$\begingroup\$ @AnaKullanıcı That's C++ code. Namespaces are a C++ thing. \$\endgroup\$ Commented Jul 20, 2022 at 13:08
  • \$\begingroup\$ namespace? where? \$\endgroup\$ Commented Jul 21, 2022 at 14:54
  • \$\begingroup\$ @AnaKullanıcı C++'s scope resolution operator :: is not a valid operator in C ;) \$\endgroup\$ Commented Jul 21, 2022 at 15:00
0
\$\begingroup\$

Naming conventions

#define uint64 unsigned long long int is misleading, as unsigned long long is only guaranteed to be at least 64 bits, while a programmer may reasonably expect uint64 to be exactly 64 bits in terms of memory usage and maybe unsigned overflow (which is guaranteed to wraparound in the C standard). C99 already gives you uint64_t in <stdint.h> which is exactly 64 bits and could be confused with your custom type.

uint64* Pow (int x, unsigned char n): I would be careful naming a function Pow because it could be confused with pow() defined in <math.h>. But you can argue the capitalization makes it clear it's different enough for programmers.

Also why is n an unsigned char? I don't think it will actually save space or run faster on a modern system with 32 or 64 bit words (I'm not sure on this). But even if you limit n to an unsigned char, it can still overflow so you don't get any safety from that.

For a toy program it's fine not to handle overflow, but I would expect documentation for any kind of arithmetic function to document how overflow is handled, if at all.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.