Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Do not write using namespace std;

Do not write using namespace std;

#Do not write using namespace std;

Do not write using namespace std;

Source Link
JDługosz
  • 11.7k
  • 19
  • 40

The include guard is too easily clashed with someone else's file when this is used. Use #pragma once; or if a guard is really needed, use a UUID.


The coType is best an enum, not a char.


Don’t write an empty destructor. Leave it off completely, or use =default to emphasize that it exists. You’ve made your class to not be trivally destructable because of this, which means the compiler can’t do certain optimizations (that's an over simplification).


Learn the canonocal way to write arithmetic operators. That is, make += a member and then write + in terms of it; etc. You don’t even have the parameter right: why can’t someone add a const value?

void foo (const Force& a, const Force& b)
{
 Force c = a + b; // fails to compile!

Your operators are assuming that the arguments are in Cartesian representation. It will get the wrong results if in polar.


Will something like

Force baz (const Force& in)
{
 Force res;
 res = baz;
 return res;
}

compile? Why not? Go over your checklist of functions you need in a class.


set_coType is silly. It is meaningless to change that without changing the existing values. Now if you had a function to change the internal representation to the chosen form, that would be better.

Having set_r1, set_r2 is not a normal thing for a simple numeric class like this. You would normally just form a new complex number, not change one of the parts of an existing value.

E.g. to get the complex conjugate of z, write Force{'C', z.r1, -z.r2};
but now you see another issue; there is no way to ensure that I’m in C format to begin with nor a way to transform it into C mode.

If we had accessors like real, imag, abs, arg then those would return the desired value whether stored or computed. E.g. if I had a polar value, real would calculate it from the polar information it has; if it was a Cartesian value, it just returns the first stored number.

I suggest modeling your complex numbers on the standard class, linked in the previous paragraph.


#Do not write using namespace std;


Your converter function that switches from one to the other representation is overly complicated, and not very useful when you can’t tell which form you start with. E.g.

void bar (const Force& f1)
{
// I want that in polar mode.
Force f2= f1;
if (/* what?? no accessor defined to check for that */)
 f2.convert();
 ⋮

Instead, have a way to convert to polar regardless of what I start with: if already in polar, it’s easy! Internally, have two separate functions with different variables; your x, y, ⋯ x, rad1 is just too muddled to deal with.

Your comment "intermeditary variables used to stop math errors in the conversion" makes me think you just added them to fix a problem. Declaring all the variables you might need at the top of a function is bad. If you declared where needed, you see you only need half of those in each branch.

And the function to do the converting should be (drum roll...) the constructor!

Force f2 {f1,'P'};

an optional second argument to the copy constructor will convert if necessary.


print should be a non-member function that is not a core part of the class. And what’s with the int return that’s always zero?

If you needed to make it a member to print coType, you should instead take that as a sign that the public API is lacking the needed features to use the class effectively (e.g. return what is the current mode).

And why not allow printing of const values?

lang-cpp

AltStyle によって変換されたページ (->オリジナル) /