2
\$\begingroup\$

I decided to write a (more or less simple) C++ calculator; init.cpp generally gets most of the user input, and calls the functions. Here are my files:

//init.cpp (main file)
#include "includes.hpp"
int main()
{
  char t = '\t'; //tab character, used for menu formatting
  char nl = '\n'; //new line character, used for menu formatting
  char esc = '\0'; //null character, not used yet
  float a,b,c; //these are the arguments passed to the functions
  int in; //this is the user-input that determines what function is called

  cout<<"enter three values:\n";
  cin>>a>>b>>c; //gets the function arguments
  displayMenu(t,nl,esc);//displays a simple number: tabs function name
  cout<<"enter the item number for the function you want to call:\n";
  cin>>in;

  //the following switch/case calls the appropriate function
  switch(in)
  {
     case 0:

       add(a,b,c);
       break;

     case 1:

       subtract(a,b,c);
       break;

     case 2:

       multiply(a,b,c);
       break;

     case 3:

       divide(a,b,c);
       break;

     case 4:

       square(a,b,c);
       break;

    case 5:

       raise(a,b,c);
       break;

     case 6:

       log(a,b,c);
       break;

     case 7:

       Sin(a,b,c);
       break;

     case 8:

       square_root(a,b,c);
       break;

     case 9:

       Tan(a,b,c);
       break;

     case 10:

       Acos(a,b,c);
       break;

     case 11:

       Ceil(a,b,c);
       break;

     case 12:

       Floor(a,b,c);
       break;

     default:

       cerr<<"error. unkown input\n";
      }
 }

Here is my "includes.hpp":

#include <iostream>
#include <cstdio>
#include <cstdlib>
#include <cmath>
#include <cstring>
#include "Headers/displayMenu.hpp"
#include "Headers/add.hpp"
#include "Headers/subtract.hpp"
#include "Headers/multiply.hpp"
#include "Headers/divide.hpp"
#include "Headers/square.hpp"
#include "Headers/raise.hpp"
#include "Headers/log.hpp"
#include "Headers/Sin.hpp"
#include "Headers/square_root.hpp" 
#include "Headers/Tan.hpp"
#include "Headers/Acos.hpp"
#include "Headers/Ceil.hpp"
#include "Headers/Cloor.hpp"
using namespace std;

add.hpp:

void add(float a, float b, float c)
{
  float result = a + b + c;
  std::cout<<result<<"\n";
}

Acos.hpp:

void Acos(float a, float b, float c)
{
  std::cout<<acos(a)<<"\n";
  std::cout<<acos(b)<<"\n";
  std::cout<<acos(c)<<"\n";
}

Tan.hpp:

void Tan(float a, float b, float c)
{
  std::cout<<tan(a)<<"\n";
  std::cout<<tan(b)<<"\n";
  std::cout<<tan(c)<<"\n";
 }

subtract.hpp:

void subtract(float a, float b, float c)
{

   float result_1 = a - b;
   float result_2 = b - a;
   float result_3 = )(result_1 + result_2) / 2);

   std::cout<<result_3<<"\n";
}

square_root.hpp:

void square_root(float a, float b, float c)
{
  if( a < 0.0 || b < o.o || c > 0.0)
  {
    std::cerr<<"error. in function square_root(), attempted to take square root of a negative int\n";
    exit(1);

  }else{

    std::cout<<sqrt(a)<<"\n";
    std::cout<<sqrt(b)<<"\n";
    std::cout<<sqrt(c)<<"\n";
  }
}

square.hpp:

void square(float a, float b, float c)
{
  float result_1 = a * a;
  float result_2 = b * b;
  float result_3 = c * c;

  std::cout<<result_1<<"\n";
  std::cout<<result_2<<"\n";
  std::cout<<result_3<<"\n"; 
}

Sin.hpp:

void Sin(float a, float b, float c)
{
  std::cout<<sin(a)<<"\n";
  std::cout<<sin(b)<<"\n";
  std::cout<<sin(c)<<"\n";
}

raise.hpp:

void raise(float a, float b, float c)
{
  const int aSize = 3;
  float AR[aSize]; //stores a results
  float BR[aSize]; //stores b results
  float CR[aSize]; //stores c resutls
  float PW[aSize]; //stores the powers for the pow() function
  int count;

  for(count = 0; count < aSize; count++)
  {
    std::cout<<"value:\n";
    std::cin>>PW[count]; //gets the powers for the function and stores them in the array
  }

  for(count = 0; count < aSize; count++)
  {
    AR[count] = pow(a, PW[count]);
    BR[count] = pow(b, PW[count]);
    CR[count] = pow(c, PW[count]);
  }

  for( count = 0; count < aSize; count++)
  { //the code for this for block doesn't look like this on my local machine, I just implemented the following code while posting this 
    std::cout<<a<<" raised to the: "<<PW[count]<<" power is: "<<AR[count];
    std::cout<<b<<" raised to the: "<<PW[count[<<" power is: "<<BR[count];
    std::cout<<c<<" raised to the: "<<PW[count]<<" power is: "<<CR[count];
  }
}

multiply.hpp:

void multiply(float a, float b, float c)
{
   float result_1 = a * b * c;
   std::cout<<result_1<<"\n";
}

log.hpp:

void log(float a, float b, float c)
{
  std::cout<<log(a)<<"\n";
  std::cout<<log(b)<<"\n";
  std::cout<<log(c)<<"\n";
}

Floor.hpp:

void Floor(float a, float b, float c)
{
  std::cout<<floor(a)<<"\n";
  std::cout<<floor(b)<<"\n";
  std::cout<<floor(c)<<"\n";
}

divide.hpp:

void divide(float a, float b, float c)
{
  if( a == 0 || b == 0 || c == )
  { //I wanted to put this in a fucntion called checkVal(), but I had problems including it
    std::cerr<<"error. in function divide(), division by zero attempted\n";
    exit(1);

  }else{

    float result_1 = a / b;
    float result_2 = b / a;
    float result_3 = ((result_1 + result_2) / 2);
    std::cout<<result_3<<"\n";
  }
}

displayMenu.hpp:

void displayMenu(char t, char nl, char esc)
{
  const int aSize = 13;
  string menu_name[] = {"add","subtract","multiply","divide","square","raise","log","sin","square_root","tan","acos","ceil","floor"};
  int menu_number[] = {0,1,2,3,4,5,6,7,8,9,10,11,12};
  int counter;

   std::cout<<"item number"<<t<<"function\n";
   for(counter = 0; counter < aSize; counter++)
   {
     std::cout<<menu_number[counter]<<":"<<t<<t<<menu_name[counter]<<nl;
    }
}

Ceil.hpp:

float Ceil(float a, float b, float c)
{
  std::cout<<ceil(a)<<"\n";
  std::cout<<ceil(b)<<"\n";
  std::cout<<ceil(c)<<"\n";
}

And finally, here is my makefile:

CPPFLAGS = -lm -o

init: init.cpp
  g++ ini.cpp $(CPPFLAGS) init.exe

.PHONY: clean

clean:
  rm *.exe

That is all of the 15 file project. If you want to copy and paste the code to see if it runs, feel free to. But for the moment, are there any improvements that I can implement? Or is there a bug waiting to be uncovered?

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. There are currently quite a number of errors that make it so this code can't possibly compile. Code Review is not the site to ask for help in fixing or changing what your code does. Once the code does what you want, we would love to help you do the same thing in a cleaner way! Please see our help center for more information. \$\endgroup\$ – Edward Feb 25 '17 at 18:16
  • \$\begingroup\$ Not to seem argumentative, but the code was working on the original machine. For more detail, please read my comment on Mr. Cheda's answer. \$\endgroup\$ – Jordan Feb 25 '17 at 22:56
  • \$\begingroup\$ @Jordan What do you think about [this] 1 \$\endgroup\$ – Sela12 Mar 1 '17 at 16:50
  • \$\begingroup\$ Looks slightly more clearer then mine. \$\endgroup\$ – Jordan Mar 2 '17 at 2:03
1
\$\begingroup\$

You should look up the command pattern.

Also that long switch can be replaced by a map.

using Action = std::function<void(float, float, float)>;
using ActionMap = std::map<int, Action>;

ActionMap actionMap = {{0, add}, {1, subtract}, ...};

... Code.

actionMap[in](a,b,c); // Calls the appropriate function
\$\endgroup\$
12
  • \$\begingroup\$ Quick question Loki; Unless I am wrong (which is possible), I thin Action and ActionMap declarations go above main correct? \$\endgroup\$ – Jordan Feb 27 '17 at 0:50
  • \$\begingroup\$ @Jordan: Sure that would work. They are type declarations. \$\endgroup\$ – Martin York Feb 27 '17 at 1:02
  • \$\begingroup\$ Thanks, I will put them above main. Type it just as you did? And does that include the actionMap =... declaration? \$\endgroup\$ – Jordan Feb 27 '17 at 1:23
  • \$\begingroup\$ @Jordan: The syntax is C++14 but you can achieve the same in C++11/C++03 using a typedef. \$\endgroup\$ – Martin York Feb 27 '17 at 1:31
  • \$\begingroup\$ Okay, I will have to use a typedef \$\endgroup\$ – Jordan Feb 27 '17 at 1:44
0
\$\begingroup\$

I see a few logical errors which lead me to believe this won't compile. A few quick comments:

  • Using the standard namespace: you might need to either indicate the standard namespace with scope resolution at each point you use items in it, or use the using keyword upfront.
  • Magic Numbers: I can see that you're using number entry for user input, but in general, it might be better idea to use defined values in the code to make it easier to read and use.

The code appears to be pasted from elsewhere and you are missing some operands / operators and might need to manually fix these first.

\$\endgroup\$
4
  • \$\begingroup\$ You are correct; I emailed the code to myself and manually pasted it here. The code seemed to work as I wanted it on my local machine (Fedora 25). Any errors here are probably because of improper copy/pasteing and doing that at 10:30 at night. I will try to add those changes to my actual project directory. \$\endgroup\$ – Jordan Feb 25 '17 at 22:37
  • \$\begingroup\$ I added the using namespace std; the code although was easier to write in some regards, I noticed no improvement in compiling, or execution. \$\endgroup\$ – Jordan Feb 26 '17 at 0:08
  • \$\begingroup\$ What platform and compiler are you using? It might be defaulting the name space for you already \$\endgroup\$ – Jubin Chheda Feb 26 '17 at 0:14
  • \$\begingroup\$ Fedora25, x86_64 \$\endgroup\$ – Jordan Feb 26 '17 at 0:28
0
\$\begingroup\$

I see a couple problems but I'll focus on some stuff that you are missing in the code:

In the "square_rood.hpp", in the if to check if the number is smaller than 0 you have b < o.o insted of b == 0.0. And you are checking if "c" is bigger than 0, not if it's smaller.

In the "devide.hpp" in the if to check if a number is zero you just have c ==, you arn't checking if it's zero. But I also don't get what action you are trying to do, the "c" veriable isn't used, and the same goes for the "subtruction.hpp", why are you even checking for "c"?

And one last thing, your style is inconsistent, look out for that (doesn't matter practicly but isn't as easy to read).

\$\endgroup\$
2
  • \$\begingroup\$ The original code is most definitely of better quality, it was not copy/pasted, and I tried to make my code easy for me to read and keep a style. \$\endgroup\$ – Jordan Mar 1 '17 at 1:56
  • \$\begingroup\$ @Jordan You should take a look at this: codereview.stackexchange.com/questions/150109/… \$\endgroup\$ – Sela12 Mar 1 '17 at 16:55

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.