[Okta Webinar] Learn how to a build a cloud-first strategyRegister Now

x
• Status: Solved
• Priority: Medium
• Security: Public
• Views: 512

# Check a student's C++

Dear all,

I would welcome feedback on the code below, I'm not sure of how to use functions or arrays but acheive the correct result doing it the way I have done it. The question is, is the way i've done it the right way.
From feedback on my previous question:
I know not to use \n but st::endl
make the comments useful but try to comment by writing understanable code
keep the text short so it can be read without scrolling across.
I'm not sure how to make it less of one bit of code and to break it up.

I am studying Electrical Engineering so C++ is a module of that, I'm not a computer programmer. Hobbyist at best with other languages (slightly)

The question set was:

Determine the voltage across the resistor at given time intervals by using the below
equation. You enter the end time and the number of time steps required; the program will
determine the voltage at each of the time steps and stored in a text file, which can be
V=Ee-(t/RC) here the E is for Applied Voltage and e is natural e not E as in exponential
Where Resistor R = 1e3 Ohms , Capacitor C = 1e-06 F, Applied voltage E = 10 volts.

The hints given are:
// writing on a text file
#include <iostream>
#include <fstream>
#include <math.h>
using namespace std;

int main ()
{
tsteps = 10; // could be different value
tend =10e-3;

ofstream myfile ("example2.txt");

// create a file  example2.txt in the notepad.

if (myfile.is_open())
{
myfile << "Time \t Voltage\n";
for (t=0; t <tend ; t+=tend/tsteps)
{
Your calculation and display output result

}
myfile.close();
}

return 0;
}

Thanks Ed.
``````/* Assignment 2 - Question 2
Ed Macey - PT EEE - Student No. 2802204
Mob. 07811 185 749 Email edmacey@gmail.com

Determine the voltage across the resistor at given time intervals by using the below
equation. You enter the end time and the number of time steps required; the program will
determine the voltage at each of the time steps and stored in a text file, which can be
Where Resistor R = 1e3 Ohms , Capacitor C = 1e-06 F, Applied voltage E = 10 volts. */

#include <iostream>
#include <stdio.h>
#include <math.h> // allows us to use more complex mathematical expressions in our programme
#include <fstream>

using namespace std;
//define all our variables and allocate values if known
double inputtime, timeintervals, nointervals, result, param, voltage;
double n = 0;
double R = 1000;
double C = 0.000001;
double E = 10;

int main()
{
//this writes the header text to data file
ofstream myfile;
myfile.open ("data.txt");
myfile << "Time interval,Voltage" << std::endl;
myfile.close();
//get input data
cout << "Voltage over time calc for a Resistive Capacitor" << std::endl;
cout << "___________________________________________________" << std::endl;
cout << "Please type in the total length of time in seconds," << std::endl;
cout << "ie 10 milliseconds would be 0.1 > ";
cin >> inputtime;
cout << "Please type in the number of time steps"  << std::endl;
cout << "you would like > ";
cin >> nointervals;
timeintervals = inputtime / nointervals;
cout << nointervals;
cout << std::endl;
cout << std::endl;
//using do while we calculate the voltage at each interval step until there
//are no more interval steps left
do
{
n=n+timeintervals;
// this is the calculation
result =(exp (-(n/(R*C))));
voltage = result*E;
cout <<  "the voltage at " << n << " seconds is " << voltage << " volts" << std::endl;
// this prints the text to data file
ofstream myfile;
myfile.open ("data.txt", ios::app);
myfile << n << "," << voltage << std::endl; // we include the comma so that matlab can
// pick up the data as a comma delimeted text file
myfile.close();

}
while (n<inputtime);

cout << std::endl;
cout << std::endl;
cout << "Please load data.txt into Matlab to plot this data" << std::endl;
return 0;
}
``````
0
edmacey
3 Solutions

Commented:
There's one basic thing that you could do better - and that is that you are opening the output file *inside* the loop, which is not a good idea. Also, a 'for' loop seems more appropriate here instead of 'do/while'. Try
``````/* Assignment 2 - Question 2
Ed Macey - PT EEE - Student No. 2802204
Mob. 07811 185 749 Email edmacey@gmail.com

Determine the voltage across the resistor at given time intervals by using the below
equation. You enter the end time and the number of time steps required; the program will
determine the voltage at each of the time steps and stored in a text file, which can be
Where Resistor R = 1e3 Ohms , Capacitor C = 1e-06 F, Applied voltage E = 10 volts. */

#include <iostream>
#include <stdio.h>
#include <math.h> // allows us to use more complex mathematical expressions in our programme
#include <fstream>

using namespace std;
//define all our variables and allocate values if known
double inputtime, timeintervals, nointervals, result, param, voltage;
double n = 0;
double R = 1000;
double C = 0.000001;
double E = 10;

int main()
{
//this writes the header text to data file
ofstream myfile;
myfile.open ("data.txt");
myfile << "Time interval,Voltage" << std::endl;
myfile.close();
//get input data
cout << "Voltage over time calc for a Resistive Capacitor" << std::endl;
cout << "___________________________________________________" << std::endl;
cout << "Please type in the total length of time in seconds," << std::endl;
cout << "ie 10 milliseconds would be 0.1 > ";
cin >> inputtime;
cout << "Please type in the number of time steps"  << std::endl;
cout << "you would like > ";
cin >> nointervals;
timeintervals = inputtime / nointervals;
cout << nointervals;
cout << std::endl;
cout << std::endl;

ofstream myfile;
myfile.open ("data.txt");

//using do while we calculate the voltage at each interval step until there
//are no more interval steps left
for (n = 0; n < inputtime; n += timeintervals)
{

// this is the calculation
result =(exp (-(n/(R*C))));
voltage = result*E;
cout <<  "the voltage at " << n << " seconds is " << voltage << " volts" << std::endl;
// this prints the text to data file
myfile << n << "," << voltage << std::endl; // we include the comma so that matlab can
// pick up the data as a comma delimeted text file
myfile.close();

}

cout << std::endl;
cout << std::endl;
cout << "Please load data.txt into Matlab to plot this data" << std::endl;
return 0;
}
``````
0

Commented:
jkr: If you open the file outside the loop, you shall close it outside also.

Constants shall be defined with const
const double R = 1000;
const double C = 0.000001;
const double E = 10;

#############################################################

The variable shall have the minor scope as possible, this allow/avoids
- avoids using the some variable in different contexts. More the once I have seen problems because of variable reuse
- ease checking of not necessary variable
- ...

#############################################################

comment shall be useful.

// this prints the text to data file
myfile << n << "," << voltage << std::endl;

Unnecessary to say the that you are printing to the data file. because is what 'myfile << ' does.

#############################################################

the comment shall always be befor the code. This makes it ease to read, and relate to code

#############################################################

#############################################################

I didn't change the comments. I'd do the code more like:
``````/* Assignment 2 - Question 2
Ed Macey - PT EEE - Student No. 2802204
Mob. 07811 185 749 Email edmacey@gmail.com

Determine the voltage across the resistor at given time intervals by using the below
equation. You enter the end time and the number of time steps required; the program will
determine the voltage at each of the time steps and stored in a text file, which can be
Where Resistor R = 1e3 Ohms , Capacitor C = 1e-06 F, Applied voltage E = 10 volts. */

#include <iostream>
#include <stdio.h>
#include <math.h> // allows us to use more complex mathematical expressions in our programme
#include <fstream>

using namespace std;

const double R = 1000;
const double C = 0.000001;
const double E = 10;

int main()
{
double inputtime, timeintervals, nointervals, param;

//get input data
cout << "Voltage over time calc for a Resistive Capacitor" << std::endl;
cout << "___________________________________________________" << std::endl;
cout << "Please type in the total length of time in seconds," << std::endl;
cout << "ie 10 milliseconds would be 0.1 > ";
cin >> inputtime;
cout << "Please type in the number of time steps"  << std::endl;
cout << "you would like > ";
cin >> nointervals;
timeintervals = inputtime / nointervals;
cout << nointervals;
cout << std::endl;
cout << std::endl;

//this writes the header text to data file
ofstream myfile;
myfile.open ("data.txt");
myfile << "Time interval,Voltage" << std::endl;

//using do while we calculate the voltage at each interval step until there
//are no more interval steps left
for (double n = 0; n < inputtime; n += timeintervals)
{
// this is the calculation
double result =(exp (-(n/(R*C))));
double voltage = result*E;
cout <<  "the voltage at " << n << " seconds is " << voltage << " volts" << std::endl;

// this prints the text to data file
myfile << n << "," << voltage << std::endl; // we include the comma so that matlab can
// pick up the data as a comma delimeted text file
}

myfile.close();

cout << std::endl;
cout << std::endl;
cout << "Please load data.txt into Matlab to plot this data" << std::endl;
return 0;
}
``````
0

Commented:
1) avoid using globally defined variables. Make them local instead (oleber already alluded to that)

2) always initialize all your variables as soon as they're defined.

3) in C++, use <cmath> and <cstdio> instead of <math.h> and <stdio.h>

4) prefer not to use 'using namespace std;'. Instead use std:: explicitly where needed

5) check the input the user gives before using it. Also check the streams after using them (especially the input stream). Related : prefer a more robust way of input by using getline rather than cin >> :

http://www.cplusplus.com/reference/string/getline/

use it to read one line of input into a string, then process the string (get the values you need out of that). This keeps the input stream in a good state, and is easier to manage.

6) open/close the file only once - you can do multiple write operations, so you can keep the file open until you no longer need it (jkr already mentioned that). Related : check whether the open succeeded or not before trying to write to the file.

7) check for division by zero, before executing a line like this :

>>         timeintervals = inputtime / nointervals;
0

Author Commented:
Thanks for this, jkr and oleber your comments were really helpful. It udnerstand why it is better to define and then use the variables as soon as you can. Infinity08 your comments too are very helpful. I've used a do while loop to ensure that the user can't input zero. I tried to use just a while loop but it wouldn't work.
I have changed the input to getline and convert the string using stringstream. However even though I've read the documentation at cplusplus I'm still not sure why getline is preferable to cin.
I tried taking using namespace std out to see what would explicitly need std:: but it threw far too many errors starting with undeclared indentifier and continuing throught the document.

So my new code (including namespace std) is below. Thanks Ed.

``````/* Assignment 2 - Question 2
Ed Macey - PT EEE - Student No. 2802204
Mob. 07811 185 749 Email edmacey@gmail.com

Determine the voltage across the resistor at given time intervals by using the below
equation. You enter the end time and the number of time steps required; the program will
determine the voltage at each of the time steps and stored in a text file, which can be
Where Resistor R = 1e3 Ohms , Capacitor C = 1e-06 F, Applied voltage E = 10 volts. */

#include <iostream>
#include <string>
#include <cstdio>
#include <cmath> // allows us to use more complex mathematical expressions in our programme
#include <fstream>
#include <sstream>

const double R = 1000;
const double C = 0.000001;
const double E = 10;

using namespace std;

int main()
{
double inputtime=0;
double timeintervals, nointervals;
double x=0;
//get input data
cout << "Voltage over time calc for a Resistive Capacitor" << std::endl;
cout << "___________________________________________________" << std::endl;

do
{
cout << "Please type in the total length of time in seconds," << std::endl;
cout << "ie 10 milliseconds would be 0.1 > ";
string mystr;
getline (cin,mystr);
stringstream(mystr) >> inputtime;
}
while (inputtime<=0);

do
{
cout << "Please type in the number of time steps"  << std::endl;
cout << "you would like > ";
cin >> nointervals;
}
while (nointervals<=0);

timeintervals = inputtime / nointervals;
cout << nointervals;
cout << std::endl;
cout << std::endl;

//this writes the header text to data file
ofstream myfile;
myfile.open ("data.txt");
myfile << "Time interval,Voltage" << std::endl;

//when the equation below is met n => inputtime then the calculation ceases
for (double n = 0; n < inputtime; n += timeintervals)
{
// this is the calculation
double result =(exp (-(n/(R*C))));
double voltage = result*E;
cout <<  "the voltage at " << n << " seconds is " << voltage << " volts" << std::endl;

// this prints the text to data file
myfile << n << "," << voltage << std::endl; // we include the comma so that matlab can
// pick up the data as a comma delimeted text file
}

myfile.close();

cout << std::endl;
cout << std::endl;
cout << "Please load data.txt into Matlab to plot this data" << std::endl;
return 0;
}
``````
0

Commented:
>> However even though I've read the documentation at cplusplus I'm still not sure why getline is preferable to cin.

The reason it's preferable is that it's more robust. If you use cin >> for input, any time there is a problem, the stream goes into an error state, forcing you to clear the error state, and handle the problem. That's not very reliable, and a lot of bother.
If you use getline to read input line by line however, things become a lot easier. Since getline simply gets all data upto a newline, and puts it into a string, there is very little chance that something goes wrong, so the stream will stay consistent. When you process a line of data from the string, and find that there's a problem with the data, all you have to do is throw away the string, and get the next line (or otherwise handle the error), but the stream will not be impacted.

>> I tried taking using namespace std out to see what would explicitly need std:: but it threw far too many errors starting with undeclared indentifier and continuing throught the document.

Well, it's still a good idea to get used to adding std:: explictly.
You'll need to add it for everything in the std namespace, like cout, end, cin, string, ofstream, etc.
0

Author Commented:
Thanks, I can see why getline is more preferable now, on presumably larger programmes.

My comprehension for std:: rather than using namespace might be similar, doesn't that just produce more work for the programmer especially on a smaller progamme like this? (Not saying I'm against it, i'm going to go through the errors now and start doing it).

Thanks Ed.
0

Commented:
If it's for small applications, or test code, it might be ok to just put in 'using namespace std;', and be done with it.

The main reason not to do it though is namespace pollution, and all the problems that result from that (the bigger the code, the more chance you'll notice adverse effects).
Sure you might have to type a bit more, but imo the advantages outweigh the disadvantages. It's up to you of course whether you want to do it for smaller applications too - I mentioned it because it's a good principle to follow that will help you avoid certain problematic situations.
0

Author Commented:
Thanks guys, sorry for not completing this earlier, have divvied the points out fairly but if there are any issues then please email me edmacey@gmail.com All very helpful. Ed.
0

## Featured Post

Tackle projects and never again get stuck behind a technical roadblock.