4
\$\begingroup\$

The following code shows the primary data structure of my calculator.

Header file contents :- theCalc.hpp

#ifndef THECALC_H_
#define THECALC_H_

#include "iostream"
#include "utility"
#include "string"
#include "cstring"
#include "cmath"

using namespace std;

class theCalc
{

protected :
    pair <float, float> inputNumberPair;
    pair <float, float> outputNumberPair;
    float flFirstNumber;
    float flSecondNumber;

public :
    void getTheInputs (string firstNumber, string secondNumber);
};


#endif

Implementation file content :- theCode.cpp

#include "theCalc.h"

void theCalc::getTheInputs(string firstNumber, string secondNumber)
{
    flFirstNumber = stof (firstNumber);
    if (!secondNumber.empty())
    {
    flSecondNumber = stof (secondNumber);
    }
    inputNumberPair = make_pair(flFirstNumber, flSecondNumber);

    cout << inputNumberPair.first << endl;
    if (!secondNumber.empty())
        {
    cout << inputNumberPair.second << endl;
        }
}

Someone to go through the code and please suggest me whether I am doing good in OOP and data structure implementation. Please suggest me is my methodologies good as a robust algorithm or I am making things complicated unnecessarily.

I am now in the learning process and need my code to be reviewed as I am a beginner.

Please take a look at the shared github the following link for the overall code.

https://github.com/arviCV/bHive2

\$\endgroup\$
3
\$\begingroup\$

Never write using namespace ... in a header. You're forcing users of your library to do something they almost certainly don't want to do.

Don't pass strings by value. Pass them by const reference (to avoid making copies) or pass them as std::(C++17) (or boost::) string_view. This also doesn't force the caller to use an actual std::string, a string_view being easily constructed even from a char buffer.

Don't mix responsibilities, separate user input from the rest of the code. Using std::cout in your class quickly becomes wrong when you change the user interface to a graphical one, or if the code ends up in a server application that only responds to network requests.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the valuable inputs, I would make necessary changes and refine the code \$\endgroup\$ Jul 31 '17 at 4:37
2
\$\begingroup\$

Design

Since the name has complex in it, I'd always think about extensibility and flexibility.

I think this design is very fragile. I'd write something like infix to postfix notation converter and evaluate the result (aka perform shunting yard algorithm).

It has multiple benefits:

  • Shunting yard can be extended with user defined functions

  • Provides possibility to deploy big ints/floats/whatever your heart desires. Of course it is not gonna be easy to implement

Some disadvantages:

  • Probably not so trivial. Last time I've written it was not pleasant experience. The infix to postfix conversion is especially painful.

  • Might have some design decision issues, as it is not a class, but an algorithm. Writing an algorithm as a class will warp its properties, and make it utterly useless in combining with other algorithms. I have worked with those, and I'll never forget how painful it was for the rest of my life.

Personally, I would ditch the class. May be other component might need to be made class, but this one doesn't seem to be so.

Code

There is a difference between <> and "" in #include. First one searches in default places, e.g. include folder for standard library, stuff specified to compiler by build system, etc. The latter relies file system. Now programmer is dealing with it directly. After learning cmake I never write those. So this point serves double duty to point at using <> and having a CMake or similar build script always.

std::endl is not a portable way to print a newline (it doesn't only print a newline). It can incur detrimental performance drop if used in a tight loop, since it is equivalent to << '\n' << std::flush. Optimal flushing is important because it is usually very slow (between ABI boundaries, to the hard drive, etc).

Storing inputs and outputs is not that efficient in my opinion. I believe std::array<> or std::vector<> would be good. Though for shunting yard probably none of those will be useful, since probably the algorithm will operate on a string.

\$\endgroup\$
1
  • \$\begingroup\$ I will try to learn and implement shunting yard algorithm. Though it seems to be scary for a beginner to master that I would love to face the challenge. I will eliminate endl and change the containers from pairs to vectors. \$\endgroup\$ Jul 31 '17 at 4:40

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.