# Basic Subnet Mask Calculator + A Little Extra

I decided to write a little "calculator" (a generous term) that when given n-many network bits, generates a 32-bit subnet mask, then displays it as octets, and as binary with the leading 0s removed. Example main:

int main() {
int nNetworkBits = 26;

printf("\n");

printf("\n");

printf("\n");
}


I'm pretty terrible at C, so I'd like comments on anything and everything that is worth mentioning. I figured out the math/bitshifting as I was writing the code, so if I'm doing anything "roundabout", I'd definitely like to know about it.

The only thing that I don't need advice on is my "abuse" of printf. I know I should be accumulating a string in a buffer then returning that buffer instead of printing directly from the functions. That's a pain in C though and detracted from the main problem that I was trying to solve, so I decided to go for the quick and dirty way.

#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>

#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;

return (BitField)0xFF << (octetI * 8);
}

int nHostBits = TOTAL_BITS - nNetworkBits;
return ~0 << nHostBits;  // TODO: Cast 0 to BitField?
}

for (int octetN = N_OCTETS - 1; octetN >= 0; octetN--) {

if (octetN > 0) {
printf(".");
}
}
}

void printBinary(BitField n) {
bool areDropping = true;  // So we can drop leading 0s.

bool isSet = n & mask;

if (isSet) {
areDropping = false;
printf("1");

} else if (!areDropping) {
printf("0");
}
}
}

int main() {
int nNetworkBits = 26;

printf("\n");

printf("\n");

printf("\n");
}


## Prefer const to #define

For constants such as TOTAL_BITS, it's generally better to use a named const value rather than a #define. The primary reason is that the const definition enforces type checking, while the macro does not. However, see the next suggestion.

## Use sizeof for portability

The code currently contains these lines:

#define TOTAL_BITS 32
#define N_OCTETS ((TOTAL_BITS / 8))

typedef uint32_t BitField;


However, I think I'd define everything instead as const values all derived from the type. That is, I'd suggest this:

typedef uint32_t BitField;
const int BitFieldOctets = sizeof(BitField);
const int BitFieldBits = 8 * BitFieldOctets;


## Think carefully about signed vs. unsigned

If generateSubnetMask is passed a negative number, what should it do? I'd suggest that the passed number should be unsigned instead of int and that the value could be checked to make sure it does not exceed the naximum number of bits.

## Be careful when shifting negative values

By default, constants are int, so this code:

return ~0 << nHostBits;


Should probably instead be written like this:

return ~0u << nHostBits;


## Avoid pointless recalculations

The code has this function:

void printOctets(BitField maskAddress) {
for (int octetN = BitFieldOctets - 1; octetN >= 0; octetN--) {

if (octetN > 0) {
printf(".");
}
}
}


This is more complex than it needs to be. First, the mask can be generated just once for the high byte and then shifted, eliminating a number of calculations that don't need to be done:

void printOctets(BitField maskAddress) {
for (unsigned i=BitFieldOctets; i; --i) {
if (i != 1) {
printf(".");
}
}
}


This works by keeping the mask in place over the most significant byte and then shifting the address to the left each iteration. An alternative would be to use a union as the BitField type to allow access to each byte very simply. That approach, however, requires that you account for the endian-ness of your machine, which this code does not.

## Consider restructuring for simplicity and clarity

The printBinary code is currently like this:

void printBinary(BitField n) {
bool areDropping = true;  // So we can drop leading 0s.

bool isSet = n & mask;

if (isSet) {
areDropping = false;
printf("1");

} else if (!areDropping) {
printf("0");
}
}
}


Instead, I'd break this into two different loops: the first to skip leading zeroes and the second to actually print the bits. Here's one way to do that:

void printBinary(BitField n) {
BitField mask = 1u << (BitFieldBits - 1);