4
\$\begingroup\$

Lately I have been playing a game called Dirty Bomb. I like it very much, and since I was taking a rest of basic C++ OpenGL, i tried to make a simple console utility for it.

What i wanted to achieve is that, when I pressed a key (representing an in-game character) the program would, after a specified delay depending on the key pressed, emit a distinct beeping. So when I see an enemy in-game character use its ability, I would press a key, and then after the cooldown delay of its ability is over(or at least near to get over), I get notified with a beeping, so i know if its a danger to get near him. I also wanted to have a "substracted delay" variable, so I could get notified a little before the enemy's ability is ready.

I wanted to have a review in the code I made, since I'm still a newbie in the world of C++ I want to know if there are ways to improve it and, if you're able, ask some questions I commented in there.

Also I've used pthreads for the beeping and waiting parts, since Beep() sleeps its calling thread and waiting can be easily done with the sleep_for() function.

Here is the code, I've commented each part so you know what each does easily:

main.cpp

#include <iostream>
#include <windows.h>    // Question: I'm only using GetAsyncKeyState and Beep functions from windows.h,
                        //           Is there a way so i don't have to include the whole windows header
                        //           and just the ones that have those functions?
#include <thread>  // for multithreading :D
#include <chrono>
#include "notes.h" // An util header i wrote to convert music notes to Hertz, so Beep() can actually play something

using namespace std;

// Holders to know which keys are and were pressed. ( so i can handle Pushing, Holding and Releasing keys ( but i just needed pushing, so :P ) )
bool lastAsyncKeyState[ 0xFF ] = {0};
bool asyncKeyState[ 0xFF ] = {0};

// Functions to save the keyboard state every cicle, so the below function can work
void saveAsyncKeyStates() { for ( int i = 0; i < 0xFF; i++ ) lastAsyncKeyState[i] = asyncKeyState[i]; }
void updateAsyncKeyStates() { for ( int i = 0; i < 0xFF; i++ ) asyncKeyState[i] = ( GetAsyncKeyState(i) ); }

// Only detect if a key was Pushed and not if it's being pressed constantly
bool asyncKeyPushed( int key ) {
    if ( asyncKeyState[key] && !lastAsyncKeyState[key] ) return true;
    return false;
}

const int updatePeriod = 10; // Delay between program updates in milliseconds
const int beepDelay = 1000;  // Delay between beeps when this program is ON
int beepCount = 0;           // counter for ON beeps
int substractDelay = 5;      // How many seconds should the program substract to the original Characters delay
int noteDuration = 200;      // Base note duration

bool shouldUpdateConsole = true; // variable to update the console only when needed
bool onSwitch = false;       // Program state ( ON/OFF )

// Enums for sound types and delays
enum SoundType {
    FRAGGER,
    JAVELIN,
    STOKER,
    SKYHAMMER,
    ONSWITCH,
    OFFSWITCH,
    ON_BEEP
};

enum Delays {
    FRAGGER_DELAY = 20,
    JAVELIN_DELAY = 30,
    STOKER_DELAY = 40,
    SKYHAMMER_DELAY = 70
};


void updateConsole() // Does what it says
{
    system("CLS"); // I know its evil, but I had no other option

    cout << endl;
    cout << "  Substracted Delay: " << substractDelay << endl << endl;
    cout << "    [ T ] Wait for FRAGGER   ( " << Delays::FRAGGER_DELAY - substractDelay << "s )" << endl;
    cout << "    [ G ] Wait for JAVELIN   ( " << Delays::JAVELIN_DELAY - substractDelay << "s )" << "  [ H ] Wait for SKYHAMMER ( " << Delays::SKYHAMMER_DELAY - substractDelay << "s )" << endl;
    cout << "    [ B ] Wait for STOKER    ( " << Delays::STOKER_DELAY - substractDelay << "s )" << endl << endl;
    cout << "  State: " << ( onSwitch ? "ON" : "OFF" ) << endl << endl;
    cout << "    Press [END] to exit." << endl;

    shouldUpdateConsole = false;
}

// Note types
void playSound( int soundType ) {
    switch( soundType ) {
        case SoundType::FRAGGER :
            Beep( noteHz(Note::A + 6*octave), noteDuration/2 );
            Beep( noteHz(Note::A + 6*octave), noteDuration );
            break;
        case SoundType::JAVELIN :
            Beep( noteHz(Note::A + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::B + 5*octave), noteDuration );
            break;
        case SoundType::STOKER :
            Beep( noteHz(Note::G + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::A + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::F + 5*octave), noteDuration );
            break;
        case SoundType::SKYHAMMER :
            Beep( noteHz(Note::G + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::A + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::G + 5*octave), noteDuration/2 );
            Beep( noteHz(Note::A + 5*octave), noteDuration );
            break;
        case SoundType::ONSWITCH :
            Beep( noteHz(Note::A + 7*octave), noteDuration/2 );
            Beep( noteHz(Note::B + 7*octave), noteDuration/2 );
            break;
        case SoundType::OFFSWITCH :
            Beep( noteHz(Note::A + 4*octave), noteDuration/2 );
            Beep( noteHz(Note::G + 4*octave), noteDuration/2 );
            break;
        case SoundType::ON_BEEP :
            Beep( noteHz(Note::C + 4*octave), noteDuration/4 );
            break;
    }
}

// Makes the current thread to wait the specified delay and then play the specified Sound
void waitForAndPlaySound( int seconds, int soundType ) {
    if ( seconds > 0 ) this_thread::sleep_for( chrono::seconds( seconds ) );
    playSound( soundType );
}

// Creates a thread that runs waitForAndPlaySound, with the specified sound and delay
void playSoundProtocol( int soundType, int delay ) {
    thread tempThread( waitForAndPlaySound , delay , soundType );
    tempThread.detach(); // Error if not detached.
}

// The main loop :D
int main()
{
    while ( !GetAsyncKeyState(VK_END) ) { // Program will execute until the END key is pressed
        updateAsyncKeyStates(); // Gets the keys that are pressed now, to compare with the keys that were pressed in the last cycle

        if ( asyncKeyPushed( VK_INSERT ) ) { // if the INSERT key is pushed, the program will switch to ON/OFF.
            onSwitch = !onSwitch;
            playSoundProtocol( ( onSwitch ? SoundType::ONSWITCH : SoundType::OFFSWITCH ), 0 );
            shouldUpdateConsole = true;
        }

        if ( asyncKeyPushed( VK_UP ) && substractDelay < 10 ) { substractDelay++; shouldUpdateConsole = true; }  // MAX substracted delay is 10
        if ( asyncKeyPushed( VK_DOWN ) && substractDelay > 0 ) { substractDelay--; shouldUpdateConsole = true; } // MIN substracted delay is 0

        if ( onSwitch ) { // If the program is ON
            if ( asyncKeyPushed( 'T' ) ) // T is to wait for the character Fragger in-game ability
                playSoundProtocol( SoundType::FRAGGER, Delays::FRAGGER_DELAY - substractDelay );
            if ( asyncKeyPushed( 'G' ) ) // G is to wait for the character Javelin in-game ability
                playSoundProtocol( SoundType::JAVELIN, Delays::JAVELIN_DELAY - substractDelay );
            if ( asyncKeyPushed( 'B' ) ) // B is to wait for the character Stoker in-game ability
                playSoundProtocol( SoundType::STOKER, Delays::STOKER_DELAY - substractDelay );
            if ( asyncKeyPushed( 'H' ) ) // H is to wait for the character Skyhammer in-game ability
                playSoundProtocol( SoundType::SKYHAMMER, Delays::SKYHAMMER_DELAY - substractDelay );

            beepCount += updatePeriod;      // The program beeps every beepDelay, so the user knows the program is active (ON).
            if ( beepCount >= beepDelay ) { beepCount -= beepDelay; playSoundProtocol( SoundType::ON_BEEP, 0 ); }

            // Question: is there a way to control the volume of Beep ?

        } else { // If the program is OFF
            if ( beepCount != 0 ) beepCount = 0;
        }

        if ( shouldUpdateConsole ) updateConsole(); // If the console should update, it calls the updateConsole func
        saveAsyncKeyStates(); // Saves the keys that are pressed now, so the next cycle can use them
        this_thread::sleep_for( chrono::milliseconds( updatePeriod ) ); // Sleeps for updatePeriod
    }
    return 0;
}

notes.h

#ifndef NOTES_H
#define NOTES_H

enum Note{ // Starts from C0
    C = -57, // C starts at -57, so A0 ( -48 ) plus 4 octaves ( 48 ) results in 0, so the noteHz function can work properly.
    Db,
    D,
    Eb,
    E,
    F,
    Gb,
    G,
    Ab,
    A,
    Bb,
    B
};

extern const int octave; // = 12

float noteHz( int );

#endif // NOTES_H

notes.cpp

#include "notes.h"
#include <cmath>

// Note to Hertz constants and stuff, more info on:
// http://pages.mtu.edu/~suits/NoteFreqCalcs.html

const float a = std::pow( 2.0f , 1.0f/12 ); // Question: if someone can tell me how to make this constant "private"
                                            // so only this .cpp can use it i would thank you :)

const int octave = 12; // Quantity of half notes in an octave ( or the length of the Note enum )

// This function transforms musical notes to Hz.
// It works by setting the center note as A4 ( 440 Hz ), then calculating every other Hz note from that point.
float noteHz( int note ) {
    return 440.0f * std::pow( a , note );
}
\$\endgroup\$
2
\$\begingroup\$
  • I understand you might have inserted some of the comments only for the sake of this review, but many of them are too verbose and in fact hurt readability. As a C++ programmer, if I see int main(), I know that's the main program without a comment, or if I see #include <thread> I know what it entails. So prefer comments that answer the question "how" instead of "why". Remember that good code is always self-commenting through proper variable names and so on.

  • Avoid writing using namespace std;. See here for more.

  • To answer your question about includes, no, you need to include a header if you need even a single function from it.

  • I would make 0xFF a constant instead of using it as a magic number.

  • Avoid using non-const global variables. Specifically, you can make beepCount, substractDelay, noteDuration, shouldUpdateConsole and onSwitch local to the main program. And moreover, is there a reason noteDuration can't be const?

  • To the previous point, updateConsole should take an argument bool shouldUpdateConsole so you don't need it to be a global.

  • You can use \n instead of std::endl for line breaks when you don't need to flush the buffer as well. See here.

  • To make a constant only visible inside a .cpp, use an unnamed namespace:

    #include "notes.h"
    #include <cmath>
    
    namespace 
    {
        const float a = std::pow( 2.0f , 1.0f/12 );
    }
    
    const int octave = 12;
    
    float noteHz( int note ) 
    {
        return 440.0f * std::pow( a , note );
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.