3
\$\begingroup\$

I've been learning a ton of c++ lately on Codecademy and I'm soon done with the course of c++.

(Still need to learn classes, object-oriented programming and harder projects.)

But I wanted to get a review of my recent program.

And I also wanted to know if its possible to make an Unique ID, that only fits me, kinda like the cicada.

Psst, the program is to solve Ohm's Formular in physics and actively takes input to solve exactly your case.

Leave suggestions on what I could do better in this project + what I should try to program next too.

//Include
#include <iostream>
#include <string>
using namespace std;

 int main(){

    //Declare variables
    double Wolt;
    double Ohm;
    double Amp;
    std::string Formular;
    std::string Username;
    std::string Whitelist;

    //Login sequence:
    std::cout << "Hvad er dit username?: ";
    cin >> Username;

    //Whitelist
    Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

    //Check if username is on whitelist
    if (Username == Whitelist){

        std::cout << "Velkommen til fysik formel udregner til strøm " << Username << " \n";
        std::cout << "Lavet af Frederik Schmidt 1.S \n";
    }

    //If username is NOT on the whitelist
    else if (Username != "Fred267s", "Seb4572s", "Popp4593s" ){

        std::cout << "Sorry, you do not have access! \n";
        return 0;
    }

    //Questions
    std::cout << "Hvad er det du skal udregne? ";
    cin >> Formular;

    //Wolt formular

    if (Formular == "Wolt"){

        //Questions
        std::cout << "Hvad er Ohmen?: ";
        cin >>Ohm;
        std::cout << "Hvad er Amperen?: ";
        cin >> Amp;

        //Formular
        Wolt = Ohm*Amp;

        //Return
        std::cout << "Volten er: " << Wolt << " \n";
    }

    //Amp formular

    else if (Formular == "Amp"){

        //Questions
        std::cout << "Hvad er Volten?: ";
        cin >>Wolt;
        std::cout << "Hvad er Ohmen?: ";
        cin >>Ohm;

        //Formular
        Amp = Wolt/Ohm;

        //Return
        std::cout << "Amperen er: " << Amp << " \n";
    }

    //Ohm formular

    else if (Formular == "Ohm"){

        //Questions
        std::cout << "Hvad er Volten?: ";
        cin >>Wolt;
        std::cout << "Hvad er Amperen?: ";
        cin >>Amp;

        //Formular 
        Ohm = Wolt/Amp;

        //Return
        cout << "Ohmen er: " << Ohm << " \n";
    }

    //If the Formular inputtet is NOT on the list.

    else {

        cout << "Error in the formular, please try again!";

    }
}
\$\endgroup\$
  • \$\begingroup\$ Wolt or Volt?:) \$\endgroup\$ – slepic Jan 7 at 12:54
  • \$\begingroup\$ @slepic Not English, so quite possibly Wolt. \$\endgroup\$ – pacmaninbw Jan 7 at 14:55
  • \$\begingroup\$ Nope, even in Norwegian it's Volt. \$\endgroup\$ – G. Sliepen Jan 7 at 19:36
  • \$\begingroup\$ Just a small comment (from a non C++ professional, though): people usually recommends against using namespace std;, so that you don't pollute your namespace with lots of stuff you probably don't need anyway. However, if you do add it, it's redundant to write code like std::cout, you can write cout only \$\endgroup\$ – jmm Jan 8 at 2:32
8
\$\begingroup\$

Don't

using namespace std;

This doesn't do what you want (only first one works):

    //Whitelist
    Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

    //Check if username is on whitelist
    if (Username == Whitelist){

[It's also a VERY dodgy access control mechanism!]

I don't like asking user for a string describing the "Formula". How does the user know they should type "Wolt"? What if they type "wolt" instead? I think it would be more reasonable to show 3 choices:

1 - Wolt    2 - Amp    3 - Ohm

Please Enter (1-3): 

And then clean the user input checking if it is a valid integer between 1-3 and then represent the choice internally using a Class enum in your code.

This will open up a range of oppotunities to use functions in your code.


enum class Formula ( wolt = 1, amp = 2, ohm = 3 );

...

Formula prompt_formula() {
  // code to read from std::cin clean input and cast to enum class
}

Then try to find a way to add even more structure to your program rather than one huge if/elseif/else statement. I am not going to give you the code, because that would defeat the purpose, but your main function should be more like this:

int main() {
  explain_usage();
  Formular form = prompt_formular();
  auto [label, answer] = compute(form);
  print_answer(label, answer);
}

I could go on, but that's probably plenty to get on with for now.

| improve this answer | |
\$\endgroup\$
7
\$\begingroup\$

Apart from Oliver Schonrock's suggestions, I'd like to add:

Be consistent

You write std::cout but cin. Either consistently prefix standard library symbols with std:: (the preferred way), or leave it out everywhere in files where you have using namespace std.

Also, be consistent in the language you are using. Some of the lines you print are in Norwegian, others in English. In fact, in some sentences you mix both.

I'd recommend you switch to English everywhere, unless there is a requirement for Norwegian variable names, comments and/or output.

Focus on one thing

Why is this program checking usernames? It makes no sense to me. If the assignment is to implement a program that calculates volts, ohms and amperes based on Ohm's law, then just focus on that.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I do not think OP has written a single Norwegian line. \$\endgroup\$ – Taemyr Jan 8 at 8:54
-1
\$\begingroup\$

People are so weird about using namespace std;. If it's a main.cpp file and you promise not to rename it like main.h to include into something else, it's fine and reduces noise. Are you seriously worried about needing to write colliding names like cout or string? It's a reasonable habit to build to qualify names since it will be necessary in larger codebases, but that rationale is premature for single file learning code.

Mixing std::cout with cin is a mistake though. To a reader, that implies there's some sort of significant difference between the two, like cin is not of the std:: namespace because otherwise why would the author specifically qualify cout?


Use a formatter like clang-format. Your main function is indented by 1 which I'm pretty sure is unintentional. You should never need to think too much about formatting in any language.


Compile your code with warnings on, e.g. -Wall. I got:

<source>: In function 'int main()':

<source>:21:41: warning: right operand of comma operator has no effect [-Wunused-value]

   21 |     Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

      |                                         ^~~~~~~~~~~

<source>:21:52: warning: right operand of comma operator has no effect [-Wunused-value]

   21 |     Whitelist = "Fred267s", "Seb4572s", "Popp4593s";

      |                                                    ^

<source>:31:50: warning: right operand of comma operator has no effect [-Wunused-value]

   31 |     else if (Username != "Fred267s", "Seb4572s", "Popp4593s" ){

      |                                                  ^~~~~~~~~~~

Which would have clued you into that issue automatically.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ The namespace std point is just about the habbit. nothing more at this point. Yes the , delimited assignment statements and comparisons were covered earlier. \$\endgroup\$ – Oliver Schönrock Jan 7 at 22:50
  • \$\begingroup\$ Yeah I saw, with warnings on the author could have caught it before sending out to review though \$\endgroup\$ – butt Jan 8 at 0:14
  • \$\begingroup\$ That's very true. I don't think he has a good setup yet. It's not that easy to do. ... \$\endgroup\$ – Oliver Schönrock Jan 8 at 0:46
  • 1
    \$\begingroup\$ One can easily "reduce noise" AND not use using namespace std; ... simply by begin specific via using std::cout; etc. \$\endgroup\$ – NichtJens Jan 8 at 12:35
  • \$\begingroup\$ Since you are a new contributor to Code Review, please note that Code Review aims for readable and production quality code. Reviewing practice code is a good opportunity to teach the OP proper programming habits. Besides, remember that Code Review, like every other Stack Exchange site, is designed to benefit the whole community, rather than just the OP. \$\endgroup\$ – L. F. Jan 14 at 10:50

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.