My First C++ App - Criticism

[Doctorate]

Hey!

I've recently started trying my hand at C++. This is my first application based on temperature conversion (original I know /s).

It would be really helpful if some experienced C++ dev could have a look at the source and comment on it's readability and cleanliness. Maybe suggest a more efficient way of doing things etc.

We've all gotten into bad coding practice at some point, and its easier to fix before it becomes a habit. :P


Also had another question: (couldn't find a definite answer on google)
- Is calling a function from another function bad practice? What if they're both in different classes?

Thanks.
The fully compiled executable is attached to this post.

Source:

// TestApp.cpp : Your road to temperature conversion!
//

#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

class temperature {
public:
	double getFahr(double cel) {
		double fahr = 1.8 * cel + 32;
		return fahr;
	}

	double getCel(double fahr) {
		double cel = fahr - 32;
		cel = cel / 1.8;
		return cel;
	}

};

class interact {
private:
	bool celcius; // If celcius is 0 then its fahr -> cel, if its 1 then its cel -> fahr conversion.

public: 
	bool verifyChoice(short choice) {
		if (choice == 1) {
			celcius = 0;
			return celcius;
		}
		else if (choice == 2) {
			celcius = 1;
			return celcius;
		}
		else {
			cout << "Error: Pick a valid choice. (1, 2)" << endl;
			exit(EXIT_FAILURE);
		}
	}
};

int main()
{

	temperature temp;
	interact inter;

	short choice;
	cout << "Please choose!" << endl << "(1) Convert Degrees Celcius to Degrees Fahrenheit." << endl << "(2) Convert Degrees Fahrenheit to Degrees Celcius." << endl << endl << "Enter your choice below!" << endl;
	cin >> choice;

	if (inter.verifyChoice(choice) == 1) {
		cout << "Initiating conversion from Degrees Fahrenheit to Degrees Celcius..." << endl << endl << "Please enter the value you wish to convert :-" << endl;
		double value;
		cin >> value;
		cout << "The conversion was successful! " << value << " degrees fahrenheit is equal to " << temp.getCel(value) << " degrees celcius.";
	}
	else {
		cout << "Initiating conversion from Degrees Celcius to Degrees Fahrenheit..." << endl << endl << "Please enter the value you wish to convert :-" << endl;
		double value;
		cin >> value;
		cout << "The conversion was successful! " << value << " degrees celcius is equal to " << temp.getFahr(value) << " degrees fahrenheit.";
	}

	cout << endl << endl << "Thank you!";

	char stay; // couldn't figure out how to keep .exe window open, so.. :P
	cin >> stay;


}

PS: Yes I understand it's bad practice to put classes and main function together, merged them for the sake of this thread.

[Yemiez]

File is safe, approved!



Feedback:

using namespace std;

It is considered bad practice to use a namespace in the global scope.

class temperature {
public:
	double getFahr(double cel) {
		double fahr = 1.8 * cel + 32;
		return fahr;
	}

	double getCel(double fahr) {
		double cel = fahr - 32;
		cel = cel / 1.8;
		return cel;
	}

};

There is really no need to put these 2 functions within a class. They dont have any members or similiar used, it would be better to have them as static functions in a namespace or similair!
e.g sizeof(temperature) = 1 (useless) byte.

celcius = 1;

Why not just true?

celcius = 1;

Why not just false?


interact basically renders useless, you could just do if (choice == 1).
I would suggest that this:

	if (inter.verifyChoice(choice) == 1) {
		cout << "Initiating conversion from Degrees Fahrenheit to Degrees Celcius..." << endl << endl << "Please enter the value you wish to convert :-" << endl;
		double value;
		cin >> value;
		cout << "The conversion was successful! " << value << " degrees fahrenheit is equal to " << temp.getCel(value) << " degrees celcius.";
	}
	else {
		cout << "Initiating conversion from Degrees Celcius to Degrees Fahrenheit..." << endl << endl << "Please enter the value you wish to convert :-" << endl;
		double value;
		cin >> value;
		cout << "The conversion was successful! " << value << " degrees celcius is equal to " << temp.getFahr(value) << " degrees fahrenheit.";
	}

Happens in the function verifyChoice, to not clutter main so much, this makes it much easier to read the main to see what happens (Which improves the readability of your code)

char stay; // couldn't figure out how to keep .exe window open, so.. :P
cin >> stay;

use

std::cin.get()

My code ( Check out my notes at the bottom for stuff to read up on! ):

enum ConvertType
{
	Celsius = -1,
	Fahrenheit = -2
};

class Converter
{
protected:
	ConvertType _type;

public:
	Converter( ConvertType t ) : _type( t ) {}

	double Convert( double val )
	{
		if ( _type == Celsius ) // Convert to celsius
			return 1.8 * val + 32;;
		return ( val / 1.8 ) - 32;
	}
};

static Converter CreateConverter(ConvertType type)
{
	return Converter(type);
}

static void DoConvertion(short choice)
{
	using ct = ConvertType; // ct = ConvertType
	auto converter = CreateConverter( ( choice == 1 ? ct( -1 ) : ct( -2 ) ) ); // If choice == 1 convert to celcius, else convert to fahrenheit
	double temp = 0.0;
	std::cout << "Enter temperature: ";
	std::cin >> temp;
	std::cout << "Result: " << converter.Convert(temp) << std::endl;
}

int main()
{
	short choice = 1;
	std::cout << "Enter choice: ";
	std::cin >> choice;
	DoConvertion(choice);
}

Compiled online with C++14

Read up on:
C++ auto variables (auto variable initializer)
C++ Conditonal operator (expr ? expr : expr)
C++ Enums

[Doctorate]

Read up on:
C++ auto variables (auto variable initializer)
C++ Conditonal operator (expr ? expr : expr)
C++ Enums

lmao I was wondering why it took you so long to post file is approved :P.
Thanks for the feedback, will definitely consider it to improve, and your example of code helps a lot. Will spend some time right now reverse engineering it.

If I'm not mistaken, I think it was your "Getting Started with Gamehacking C++" thread (the one that recommended Coding Newbie and thenewboston) that I read when I started learning.
Thanks a lot overall You've been an incredible help.

[Tullingen]

Hey

Here is an alternative method of using it (runtime is about the same), but its considered good practise to use lambdas because it creates expressive code.

#include <iostream>

int main(){
    short choice = 0;
    std::cout << "Enter choise: ";
    std::cin >> choice;
    std::cout << choice << std::endl;
    
    float temprature = 0;
    std::cout << "Enter temprature: ";
    std::cin >> temprature;
    std::cout << temprature << std::endl;
    
    auto converted = [=](){
        if ( choice == Celsius ) // Convert to celsius
            return 1.8 * temprature + 32;
        return ( temprature / 1.8 ) - 32;
    };
    
    std::cout << converted() << std::endl;
    return 0;
}

I cant post links, but just google "c++ lambda introduction" or something

[maestro1994]

using namespace std;

It is considered bad practice to use a namespace in the global scope.

using namespace std; is ok used in a translation unit. It's bad **if** used on header files as all us are expected to not know where your header file will be included.