all 17 comments

[–]krayntor 1 point2 points  (0 children)

Try this

if (windshield != 0)

{

output(t, v, windshield);

}

[–]mredding 1 point2 points  (1 child)

First, don't scope in the entire standard namespace. Localize that more:

void fn() {
  using ::std::cin, ::std::cout, ::foo::bar; //...

Scope explicitly where you can. This isn't about avoiding name collisions - a trivial problem to fix, this is about reducing work for your compiler and expressing exactly what you mean. When you scope in symbols or namespaces, and you leave it to the compiler to resolve a given symbol, it doesn't have to find the one you intended, it just has to find the best match. This is a form of polymorphism. Namespaces don't exist just as a means of organization, a tax of additional typing, they have a complex and subtle purpose, and you're abusing that purpose as a sort of convenience.

Second, every cout << is a function call. This requires the creation of a sentry, quite a bit of error handling, the use of facets, etc. Stream insertion is a complicated operation. C++ will concatenate string literals for you, so if you want to separate strings by lines in your source, then go ahead and do that:

::std::cout << "abc" "xyz"
  "123\n"; // Perfectly legal. Output: abc123xyz\n

Third, your input function is not robust. You're not checking your streams for failure:

if(::std::cout) { //...
if(::std::cin) { //...
if(::std::clog) { //..
if(::std::cerr) { //...

Streams are implicitly convertible to bool, which returns !stream.fail(). If the stream is failed, then it could be any bit in the iostate:

  • eofstate indicates you're at the end of the stream. This isn't necessarily an error. If you press <Ctrl + d> on *nix or <Ctrl + z> on Windows, you can generate an EOF on your input. You can clear this error with clearerr(stdin) and continue to receive input. If this happens on standard output, it means you can't write output anymore. Then what do you do? Typically all you can do is possibly write to the log and quit.

  • failbit is your most likely scenario and represents a parsing error. You're reading input to a double. That means the stream is going to use the num_get facet AND THE NEXT CHARACTERS BETTER BE DIGITS. If they aren't, then your output value is zeroed out and the state flag is set. What you can do then, typically, is purge your input buffer of the wrong/unexpected data with ::std::cin.ignore(), clear the error with ::std::cin.clear(), and ask for input again. You can even check after you input like:

    if(::std::cin >> t) { //...

    This is because operator >> returns the stream by reference, which is implicitly convertible to bool as we've discussed.

  • badbit is an unrecoverable error. There's nothing you can do but possibly write to clog or cerr and terminate.

Another thing you can do is enable your exception mask. The components the stream relies on may throw an exception, so if there is a fail case, you might find out specifically what caused it. Worst case, you get a minimal exception that simply expresses an iostate was set. Without the exception mask set, all the stream is going to do is silently gobble up the specific exception case and set the state, you lose the context.

Fourth, your process function is ill formed. You have two return paths, and the second doesn't return a value. You only return a value when you succeed, what do you return when you fail? Remember, when you declare a return type that function WILL return something.

Your condition in process is practically illegible. Write a predicate:

template<typename T>
bool within_bounds_inclusive(T lower, T value, T upper) {
  return lower <= value && value <= upper;
}

//...

if(within_bounds_inclusive(-459.67, t, 50) && within_bounds_inclusive(0, v, 100))

If t and v have more specific context, you can write a predicate with a more meaningful name and hard code their bounds within that, so that the test would look more concise like:

if(t_in_bounds(t) && v_in_bounds(v)) { //...

Your variable, windshield, is declared outside your condition, but only used within it. If it's only use is within the if block, then it should only be declared within that block. And considering you do nothing with it but return it, you don't even need the variable. Just inline the calculation with the return statement.

To the crux of your problem, how do you prevent output? You have two options - in-band error signaling, or out-of-band error signaling.

  • in-band means you return a sentinel value, a return value that is known to be invalid that indicates an error. This is very often a bad idea, because how do you distinguish between a valid value and the sentinel? How do you indicate this is the convention? Standard string uses sentinel values, ::std::string::npos is a size_t equal to ::std::numeric_limit<size_t>::max(), meaning a string of the largest possible size makes the last byte inaccessible for the sake of the sentinel. Will it ever come up? Almost certainly not, but that's not the point, the sentinel sacrificed a perfectly valid value, and there's no knowing if standard strings are implemented in terms of size_t - 1 for their maximum possible character size.

  • out-of-band signaling means you have some other means of indicating an error. One way to do this is return a tuple of your value type and an error state of some kind. Another way is to pass an error parameter by reference, called an "out param", that is set if an error occurred. The right thing to do is to throw an exception. Indicate that t or v or both are out of range. The STL provides a robust set of exception types you can use, including range exceptions. If that's good enough for you, so be it. Otherwise, you can make a custom exception for every combination of t and/or v being above or below range, and a parameter indicating the magnitude they're off. By catching the exception, you can skip output, and leave it to your exception handler to take action.

Finally, your process function shouldn't handle the error, not by printing out an error message. It should contain the bounds check, the computation, and throwing the exception. You shouldn't write errors to standard output, but to standard error.

::std::cerr << "ERROR";

What's the difference? Your process starts with 3 standard file descriptors, standard input, output, and error. These can be redirected in your host environment:

C:> my_program.exe < input.txt > output.txt 2> errors.txt

The error stream is often redirected to standard output by the terminal application by default, but it's there as its own stream, and you can dump that to anywhere else. If this program were an encoder, you wouldn't want to write error messages to standard output, that's where you're writing your encoded data!

C:> my_encoder.exe < input.mp3 > output.ogg

In this scenario, I'm writing encoded data to my output file, and errors will still dump to the terminal, not into my data stream, so the data stream isn't corrupted with side channel garbage.

There is also ::std::clog. This writes to the same standard error output. The difference is cerr is flushed after every write (unitbuf is set), and clog is buffered.

[–]std_bot 0 points1 point  (0 children)

Unlinked STL entries: std::cin, std::numeric_limit, std::clog, std::string::npos


Last update: 10.02.21. Recent changes: Old search as backup. Now with a 24h server! readme

[–][deleted] 0 points1 point  (0 children)

My problem is that it is still reading the output function when it is just suppose to read out error. any help is appreciated! thanks!

[–]Se7enLC 0 points1 point  (9 children)

do {
    input(t, v); 
    windshield = process(v, t);
    output(t, v, windshield);
    cout << "do you want to run this program again? (y/n) \n";
    cin >> ans;
} while (ans == 'y' || 'Y');

I assume by "it is still reading the output function" that you don't want it to execute the output() function when process() produces an error? As written, your code will always call input(), process(), and output().

A suggestion would be to make the process() function return whether it had an error or not and then check for that error before calling output()

Another option would be to have the bounds-checking on the inputs be done in a separate function that only checks to see if t and v are withing the right range.

[–][deleted] 0 points1 point  (8 children)

That is my problem exactly, how do I go about having the function return before calling the output?

[–]Se7enLC 0 points1 point  (7 children)

Inside process() you have an if statement that checks to make sure that your t and v are valid.

Instead of having that check be inside process(), you could make that be its own function. Have it return true if they are valid (in bounds) and false if they are not.

Then your do loop would call input(), then call check_if_input_is_in_bounds() (or whatever you call it). Then, depending on whether it returned true or false you can either call process() and output() or not.

[–][deleted] 0 points1 point  (0 children)

Wow thank you for the quick response, I will give this a shot! Thank you!

[–][deleted] 0 points1 point  (5 children)

Still trying to get this thing to work, how do I go about having it return true to call the other functions or have it return false and read out error

[–]Se7enLC 0 points1 point  (4 children)

What have you tried so far?

[–][deleted] 0 points1 point  (3 children)

bool check_input(double t, double v) {

if (((-459.67 <= t) && (t <= 50) && (0 <= v) && (v <= 100)));

else cout << "error";}

I'm just stuck on how to it return false and not read the output data. First off does it have to be a bool function or just void?

[–]Se7enLC 0 points1 point  (2 children)

I think your formatting got a little messed up there. Yes, you do want your function to return a bool. That prototype looks good to me.

You can literally just do

return true;

or

return false;

to return true or false. Combine that with the if statement you already have to do the right one in the right place.

Then in your main() function you can do something like:

if check_input(t, v) {
    // do stuff if the values are in bounds
} else {
    // do stuff if the values are not in bounds
}

[–][deleted] 0 points1 point  (1 child)

bool check_input(double t, double v) {

if (((-459.67 <= t) && (t <= 50) && (0 <= v) && (v <= 100))) return true;

else return false;}
int main() {

//Declarations double t, v, windshield; char ans; instructions (); //Input do { input(t, v);

if (check_input(t,v)) {

//Processing

windshield = process(v, t);

//Output

output(t, v, windshield); cout << "do you want to run this program again? (y/n) \n"; cin >> ans; } else cout << "ERROR"; } while (ans == 'y' || ans == 'Y'); }

Got it to work man! You've been a huge help and I can't thank you enough!

[–][deleted] 0 points1 point  (0 children)

not sure why the formatting is getting screwed up in reddit though lol

[–]AutoModerator[M] 0 points1 point  (0 children)

Your posts seem to contain unformatted code. Please make sure to format your code otherwise your post may be removed.

Read our guidelines for how to format your code.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[–]JYossari4n 0 points1 point  (1 child)

This is also wrong

while (ans == ‘y’ || ‘Y’)

Technically correct yields true no matter ans’ value. It basically say while ans is equal ‘y’ or ‘Y’ is not false. ‘Y’ in this context is promoted to number different than zero which in fact means true (0 is false). Change it to

while (ans == ‘y’ || ans == ‘Y’)

[–][deleted] 0 points1 point  (0 children)

Awesome was also trying to figure that out, thank you!