Second opinion needed on C structure program that models an ideal transformer

Hi guys. I need a second opinion on another C program that I am working on that models an ideal transformer. The program requirements are as follows:

Design and implement a structure type to model an ideal transformer. If you have a single iron core with wire 1 coiled aorund the core N1 times and wire 2 wound around the core N2 times, and if wire 1 is attached to a source of alternating current, then the voltage in wire 1 (the input voltage v1) is related to the voltage in wire 2 (the output voltage v2) as:

V1 = N2V2 N2

and the relationship between the input current I1 and the output current I2 is

I1 = N2I2 N1

A variable of type transformer_t should store N1, N2, V1, and I1. Also define functions v_out and i_out to compute the output voltage and current of a transformer. In addition, define functions that set each of the transformer's components to produce a desired output voltage or current. For example, function set_n1_for_v2 should take a desired output voltage as an input parameter and a transformer as an input/output parameter and should change the component representing N1 to produce the desired current. Also define set_v1_for_v2, set_n2_for_v2, and set_n2_for_i2. Include scan_transformer and print_transformer functions to facilitate I/O.

Here is my code for this problem:

Code:

#include <stdio.h>

typedef struct{ int n1, /* The number of times the first wire is wrapped around the transformer */ n2; /* The number of times the second wire is wrapped around the transformer */ double v1, /* The input voltage value */ i1; /* The input current value */} transformer_t; /* Structure of electrical currents in a transformer */

double v_out(double, int, int); /* Computes the output voltage of a transformer */double i_out(double, int, int); /* Computes the output current of a transformer */transformer_t set_n1_for_v2(transformer_t *, double); /* Set the number of primary wiring to produce the desired voltage for v2 */transformer_t set_v1_for_v2(transformer_t *, double); /* Set the primary voltage to produce the desired voltage for v2 */transformer_t set_n2_for_v2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired voltage for v2 */transformer_t set_n2_for_i2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired current for i2 */int scan_transformer(transformer_t *); /* Gets values for the transformer */void print_transformer(transformer_t); /* Prints calculated values for the transformer */

/* Verify if all structure input values have been entered */ if (result == 4) result = 1; else if (result != EOF) result = 0;

return result; }

void print_transformer(transformer_t tranp){ /* Display the desired result */ printf("\nThe desired number of windings for the first wire is %d", tranp.n1); printf("\nThe desired number of windings for the second wire is %d", tranp.n2); printf("\nThe desired input voltage is %.1f", tranp.v1); printf("\nThe desired input current is %.1f\n", tranp.i1); }

A variable of type transformer_t should store N1, N2, V1, and I1. Also define functions v_out and i_out to compute the output voltage and current of a transformer. In addition, define functions that set each of the transformer's components to produce a desired output voltage or current. For example, function set_n1_for_v2 should take a desired output voltage as an input parameter and a transformer as an input/output parameter and should change the component representing N1 to produce the desired current. Also define set_v1_for_v2, set_n2_for_v2, and set_n2_for_i2. Include scan_transformer and print_transformer functions to facilitate I/O.

I dislike your commenting style (/* Declare variables */ - really?) but whatever. In my opinion the more important weakness is in your requirements analysis & how you built your based off of that.

Thoughts: - What does a negative value of n1 or n2 represent? - When calculating the voltage/current output of a transformer_t, why not take a transformer_t as an argument? - The prototypes of your set_FOO_for_BAR functions seem quite confused. Think about what you're passing in & what you're doing with it. Why are you passing in a variable and then immediately doing I/O to overwrite it? What are you returning & why? - Passing or returning structures by value is often considered poor form. Commenting everything under the sun but NOT explaining what/why you're returning is ALWAYS poor form. - What does the return value of scan_transformer represent?

- You might get quicker/better results on StackOverflow. Especially if you ask focused questions rather than just asking for a free code review.

Okay, after fiddling around with this program, I have a more improved version of it. Here is the code:

Code:

#include <stdio.h>

typedef struct{ double n1, /* The number of times the first wire is wrapped around the transformer */ n2, /* The number of times the second wire is wrapped around the transformer */ v1, /* The input voltage value */ i1; /* The input current value */} transformer_t; /* Structure of electrical currents in a transformer */

double v_out(double, int, int); /* Computes the output voltage of a transformer */double i_out(double, int, int); /* Computes the output current of a transformer */transformer_t set_n1_for_v2(transformer_t *, double); /* Set the number of primary wiring to produce the desired voltage for v2 */transformer_t set_v1_for_v2(transformer_t *, double); /* Set the primary voltage to produce the desired voltage for v2 */transformer_t set_n2_for_v2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired voltage for v2 */transformer_t set_n2_for_i2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired current for i2 */int scan_transformer(transformer_t *); /* Gets values for the transformer */void print_transformer(transformer_t); /* Prints calculated values for the transformer */

/* Verify if all structure input values have been entered */ if (result == 4) result = 1; else if (result != EOF) result = 0;

return result; }

void print_transformer(transformer_t tranp){ printf("\nThe desired number of windings for the first wire is %.0f", tranp.n1); printf("\nThe desired number of windings for the second wire is %.0f", tranp.n2); printf("\nThe desired input voltage is %.1f", tranp.v1); printf("\nThe desired input current is %.1f\n", tranp.i1); }

I dislike your commenting style (/* Declare variables */ - really?) but whatever. In my opinion the more important weakness is in your requirements analysis & how you built your program based off of that.

All irrelevant comments have been removed.

Quote:

- What does a negative value of n1 or n2 represent?

The program was modified to prompt the user over and over to enter a value of n1 or n2 greater than or equal to 0 should the user enter a negative value. n1 and n2 are supposed to represent a physical coiling of a wire around an iron core.

Quote:

- When calculating the voltage/current output of a transformer_t, why not take a transformer_t as an argument?

The program requirements put forth by professors Jeri R. Hanly and Elliot B. Koffman do not allow output current and output voltage to be used in transformer_t. I'm only abiding by the requirements put forth by both professors.

Quote:

- The prototypes of your set_FOO_for_BAR functions seem quite confused. Think about what you're passing in & what you're doing with it. Why are you passing in a variable and then immediately doing I/O to overwrite it? What are you returning & why?

The program was modified to take in the original variables rather than copies of said variables. Now the functions return the new desired values for the original variables rather than the copies.

Quote:

Passing or returning structures by value is often considered poor form. Commenting everything under the sun but NOT explaining what/why you're returning is ALWAYS poor form.

Point duly noted for future group projects.

Quote:

- What does the return value of scan_transformer represent?

It is an error check value for structure inputs that is preferred by professors Jeri R. Hanly and Elliot B. Koffman. If all relevant input from scan_transformer has been met, the result value will be 1 to indicate success. Otherwise, a value of 0 is returned to indicate an error.

Quote:

- You might get quicker/better results on StackOverflow. Especially if you ask focused questions rather than just asking for a free code review.

It is an error check value for structure inputs that is preferred by professors Jeri R. Hanly and Elliot B. Koffman. If all relevant input from scan_transformer has been met, the result value will be 1 to indicate success. Otherwise, a value of 0 is returned to indicate an error.Quote:

This is backwards from the rest of the C-programming world when you're just indicating success versus failure - 0 should be success, and non-zero failure. That leaves the possibility of different non-zero error codes to indicate what went wrong. If zero indicates failure, you've only got one possible value.

In more complicated cases, you have things like returning negative integers on failure, and zero or positive on non-failure to indicate something about the result (e.g. bytes written, FD opened).

This is backwards from the rest of the C-programming world when you're just indicating success versus failure - 0 should be success, and non-zero failure. That leaves the possibility of different non-zero error codes to indicate what went wrong. If zero indicates failure, you've only got one possible value.

Then why would two university professors use this form of error check if it is considered backwards thinking by the rest of the C-programming world? Both Hanly and Koffman published programming books that insist on this type of error checking and they are both experts in the field of software engineering. Why would they be wrong?

This is backwards from the rest of the C-programming world when you're just indicating success versus failure - 0 should be success, and non-zero failure. That leaves the possibility of different non-zero error codes to indicate what went wrong. If zero indicates failure, you've only got one possible value.

Then why would two university professors use this form of error check if it is considered backwards thinking by the rest of the C-programming world?

This is backwards from the rest of the C-programming world when you're just indicating success versus failure - 0 should be success, and non-zero failure.

To be fair the C programming world isn't exactly consistent on this front. True, UNIXy calls tend to return 0 on success, but malloc() and fopen() don't. Functions return what the documentation says they return to indicate success or failure.

typedef struct{ double n1, /* The number of times the first wire is wrapped around the transformer */ n2, /* The number of times the second wire is wrapped around the transformer */ v1, /* The input voltage value */ i1; /* The input current value */} transformer_t; /* Structure of electrical currents in a transformer */

double v_out(double, int, int); /* Computes the output voltage of a transformer */double i_out(double, int, int); /* Computes the output current of a transformer */transformer_t set_n1_for_v2(transformer_t *, double); /* Set the number of primary wiring to produce the desired voltage for v2 */transformer_t set_v1_for_v2(transformer_t *, double); /* Set the primary voltage to produce the desired voltage for v2 */transformer_t set_n2_for_v2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired voltage for v2 */transformer_t set_n2_for_i2(transformer_t *, double); /* Set the number of secondary wiring to produce the desired current for i2 */int scan_transformer(transformer_t *); /* Gets values for the transformer */void print_transformer(transformer_t); /* Prints calculated values for the transformer */

/* Verify if all structure input values have been entered */ if (result == 4) result = 0; else if (result != EOF) result = 1;

return result; }

void print_transformer(transformer_t tranp){ printf("\nThe desired number of windings for the first wire is %.0f", tranp.n1); printf("\nThe desired number of windings for the second wire is %.0f", tranp.n2); printf("\nThe desired input voltage is %.1f", tranp.v1); printf("\nThe desired input current is %.1f\n", tranp.i1); }

typedef struct{ double n1, /* The number of times the first wire is wrapped around the transformer */ n2, /* The number of times the second wire is wrapped around the transformer */ v1, /* The input voltage value */ i1; /* The input current value */} transformer_t; /* Structure of electrical currents in a transformer */

Why not give the variables meaningful names? Autocomplete FTW and you don't need to comment the cryptic minimalism. And declare the struct members separately.

Why do these take a bunch of numeric arguments instead of a transformer_t*? If you're going to keep the arguments as-is, at least specify what the inputs are in the description. Also, include the argument names in the prototype. No, it's not required for the code to compile but it makes it easier for people to understand.

Quote:

Code:

/* Set the number of primary wiring ... */

When you say "wiring", I think you mean "windings" or "turns in the <primary or secondary> winding".

The logic of surrounding "result" in scan_transformer makes no sense at all. What are you trying to do, in words?

This is backwards from the rest of the C-programming world when you're just indicating success versus failure - 0 should be success, and non-zero failure.

To be fair the C programming world isn't exactly consistent on this front. True, UNIXy calls tend to return 0 on success, but malloc() and fopen() don't. Functions return what the documentation says they return to indicate success or failure.

^ This, exactly.

There are (at least) three different schools of thought, all of which you'll find somewhere inside the C library of pretty much any "modern" operating system:

(1) Return 0 on success; return non-zero on failure with the value indicating the type of failure.(2) Return 1 (or some "meaningful" value like a pointer) on success; return 0 on failure; use errno to indicate the type of failure (lets you do things like 'if(doOperation()) { stuffOnSuccess(); } else { handleFailure(); }' ).(3) Return >= 1 under normal operation (value typically indicating something to the effect of the amount of data consumed); return 0 on some "normal" terminating condition; return -1 on error with errno indicating the type of failure.

The moral of this story: (1) read the documentation for APIs you're consuming and (2) document the expected behavior of things you write. And do try to be consistent, at least.

void PrintTransformer(Transformer TranPointer){ printf("\nThe desired number of windings for the first wire is %.0f", TranPointer.PrimaryWire); printf("\nThe desired number of windings for the second wire is %.0f", TranPointer.SecondaryWire); printf("\nThe desired input voltage is %.1f", TranPointer.InputVoltage); printf("\nThe desired input current is %.1f\n", TranPointer.InputCurrent); }

Many of your functions, e.g. GetOutputVoltage, take a pointer to a Transformer struct, modify that struct, and then return the (now modified) struct by value. Why?

The names are also misleading: you call your functions GetXXX and SetXXX, but 'Get' and 'Set' don't appear to carry any meaning, because both function-types mutate the passed argument.

If your functions implement a particular formula, it makes a lot more sense to me to have them take specifically the parameters the formula requires, and do the assignment in the outer scope. For example,

printf("\n\nEnter new output voltage towards the calculation of the new primary wire value: "); scanf("%lf", &Tran.OutputVoltage); Tran.PrimaryWire = SetFirstWireWithOutputVoltage(Tran.InputVoltage, Tran.SecondaryWire, Tran.OutputVoltage); printf("\nThe desired number of windings for the first wire is %.0f", Tran.PrimaryWire);

printf("\n\nEnter new output voltage towards the calculation of the new secondary wire value: "); scanf("%lf", &Tran.OutputVoltage); Tran.SecondaryWire = SetSecondWireWithOutputVoltage(Tran.OutputVoltage, Tran.PrimaryWire, Tran.InputVoltage); printf("\nThe desired number of windings for the second wire is %.0f", Tran.SecondaryWire);

printf("\n\nEnter new output current towards the calculation of the new secondary wire value: "); scanf("%lf", &Tran.OutputCurrent); Tran.SecondaryWire = SetSecondWireWithOutputCurrent(Tran.InputCurrent, Tran.PrimaryWire, Tran.OutputCurrent); printf("\nThe desired number of windings for the second wire is %.0f\n", Tran.SecondaryWire);

I had a quick glance at the program, and while I cannot say anything on the usage of the struct and pointers or the formulas (that would need more time), I do see an issue with getting the input from the user.

You're using scanf() everywhere, and in my experience, scanf() is not reliable for getting input that may not be valid. For example, in all those places where you're getting a numeric input through scanf(), have you tested those by entering plain alphabetic strings? I've observed that entering a string for such cases, or even entering a longer input than what's expected, would cause the program to crash. A better approach is to use gets() to get a string input, validate it yourself for the type of data you need (using functions like strtol(), strtof() and so on) and display an error message to the user if necessary. Put the following in a loop for every input - gets(), validate and continue looping on error; exit loop on valid input.

You're using scanf() everywhere, and in my experience, scanf() is not reliable for getting input that may not be valid. For example, in all those places where you're getting a numeric input through scanf(), have you tested those by entering plain alphabetic strings? I've observed that entering a string for such cases, or even entering a longer input than what's expected, would cause the program to crash. A better approach is to use gets() to get a string input, validate it yourself for the type of data you need (using functions like strtol(), strtof() and so on) and display an error message to the user if necessary. Put the following in a loop for every input - gets(), validate and continue looping on error; exit loop on valid input.

Okay, I'm sorry that I have to say this but I am honestly not going to follow this advice. I've run the program more times than I can possibly count and I can attest to the fact that the calculations all check out for me.

I only put up with the advice that was given to me so far on the grounds that I can finally have a program that will meet everyone's standards here. Unfortunately, that does not appear to be the case here.

This program is not meant to be part of a larger, group project. This is nothing but a tiny personal homework assignment designed to teach how structures work, nothing more, nothing less. While I did appreciate everyone's advice up to now, I don't see anything else that warrants further revision other than to satisfy one member's personal preferences while risk alienating another member's personal preferences.

To that, I humbly ask that we finally put this program to rest. The program works. I'm happy. The professors are happy. We're all happy. I can also speak for everyone else that we all have better things to do than to deal with a program by a forum newbie.

I had a quick glance at the program, and while I cannot say anything on the usage of the struct and pointers or the formulas (that would need more time), I do see an issue with getting the input from the user.

You're using scanf() everywhere, and in my experience, scanf() is not reliable for getting input that may not be valid. For example, in all those places where you're getting a numeric input through scanf(), have you tested those by entering plain alphabetic strings? I've observed that entering a string for such cases, or even entering a longer input than what's expected, would cause the program to crash. A better approach is to use gets() to get a string input, validate it yourself for the type of data you need (using functions like strtol(), strtof() and so on) and display an error message to the user if necessary. Put the following in a loop for every input - gets(), validate and continue looping on error; exit loop on valid input.

No, no, no. gets() is a stability and security disaster waiting to happen, because it reads as much input as it can see, with no regard for the space available for storing it. It's so bad that it was deprecated in the C99 standard, and removed in the C11 standard. Its use should never be recommended to anyone, especially not a new programmer who won't recognize that someone has handed them a ticking bomb. If you want to read a whole line and parse it afterwards, use fgets or gets_s instead.

No, no, no. gets() is a stability and security disaster waiting to happen, because it reads as much input as it can see, with no regard for the space available for storing it. It's so bad that it was deprecated in the C99 standard, and removed in the C11 standard. Its use should never be recommended to anyone, especially not a new programmer who won't recognize that someone has handed them a ticking bomb. If you want to read a whole line and parse it afterwards, use fgets or gets_s instead.

Thanks. I understand your recommendation, and will not recommend gets() again (and instead have fgets() or gets_s() take its place).