Siplifying code

Hello I have part in my code which is repeating and I would like to change it to function. Can you help me please?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
 typedef struct {
	int citatel;
	int menovatel;
} zlomek;
zlomek A = { 0,1};
zlomek B = { 0,0};


zlomek zjednodusenie(int a, int b) {
	int i;
	if (a == 0){
		zlomek ab = { a,b };
		return ab;
	}
	else if (b == 0) {
		zlomek ab = { 0,0 };
		return ab;
	}
	for (i = b; i > 1; i--) {
		if ((a % i == 0) && (b % i == 0)) {
			a /= i;
			b /= i;
		}
	}
	zlomek ab = { a,b };
	return ab;
}
zlomek soucet(zlomek p1, zlomek p2) {
//this part is repeating in every function that i have, I want to change it to a function 
	/*if (p1.menovatel == 0 || p2.menovatel == 0) {
		zlomek ab = { 0,0 };
		return ab;
	}
	else */{
		int cit, men;
		men = p1.menovatel * p2.menovatel;
		cit = ((men / p1.menovatel)*p1.citatel) + ((men / p2.menovatel)*p2.citatel);
		return zjednodusenie(cit, men);
	}
	}	
how about..

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
bool  zafu(zlomek p1, zlomek p2, zlomek &ab)
  {
    zlomek tmp { 0,0 };
     if (p1.menovatel == 0 || p2.menovatel == 0) 
        {
	  ab = tmp;
	return true;
	}
      return false;
   }


zlomek soucet(zlomek p1, zlomek p2) 
{
        zlomek ab;
	if(zafu(p1, p2, ab))
          return ab;
	else 
          {
		int cit, men;
		men = p1.menovatel * p2.menovatel;
		cit = ((men / p1.menovatel)*p1.citatel) + ((men / p2.menovatel)*p2.citatel);
  	 return zjednodusenie(cit, men);
	}
}


this seems like a lot of trouble though.
a constant ab set to {0,0} in your namespace or class, defined one time, would let you just do this, without the function call or extra variables and general mess:

if(condition)
return ab;
else
{
other code
}

you can make the condition a constant expression, I think. I am still working on those. Then you could change it once if you ever needed to change it everywhere....
Last edited on
Adding a function like
bool noZeroDivision(zlomek p1, zlomek p2) { return p1.menovatel != 0 && p2.menovatel != 0; }
could make your code more DRY-compliant, but in my opinion if you decided to switch your code to C++, you could take advantage of encapsulation - which doesn’t mean your code would become shorter.
The crux in your code is a variable should not being assigned a specific value: encapsulation gives you a chance to prevent it from happening rather than just checking if it has happened.

Basic example:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
#include <iostream>
#include <limits>

class Zlomek {
public:
    int citatel;
    bool good;
    Zlomek(int cit_arg = -1, int men_arg = -1) : citatel { cit_arg }
    {
        if(men_arg == 0) {
            good = false;
            citatel = menovatel = -1;
        } else {
            good = true;
            menovatel = men_arg;
        }
    }
    int getMenovatel() const { return menovatel; }
    Zlomek& zjednodusenie()
    {
        if (citatel == 0) { return *this; }
        for (int i = menovatel; i > 1; i--) {
            if ((citatel % i == 0) && (menovatel % i == 0)) {
                citatel /= i;
                menovatel /= i;
            }
        }
        // This passage is debatable:
        if(citatel < 0 && menovatel < 0) {
            citatel *= -1;
            menovatel *= -1;
        }
        return *this;
    }
    Zlomek& soucet(const Zlomek& other)
    {
        int men = menovatel * other.menovatel;
        int cit =   ( (men / menovatel) * citatel)
                  + ( (men / other.menovatel) * other.citatel);
        menovatel = men;
        citatel = cit;
        return zjednodusenie();
    }
    Zlomek& operator+=(const Zlomek& rhs) { return soucet(rhs); }
    friend Zlomek operator+(Zlomek lhs, const Zlomek& rhs)
    {
        lhs += rhs;
        return lhs;
    }
    friend std::ostream& operator<<(std::ostream& os, const Zlomek& rhs)
    {
        os << rhs.citatel;
        if(rhs.menovatel != 1) { os << '/' << rhs.menovatel; }
        return os;
    }
private:
    int menovatel;
};

void waitForEnter();

int main()
{
    Zlomek a = { 0, 1 };
    Zlomek b = { 0, 0 };
    std::cout << "a: " << a << "; b: " << b << '\n';
    if(!a.good || !b.good) {
        std::cout << "Couldn't initialise one of the two zlomek.\n";
    }
    Zlomek c { 26, 13 }, d { 81, 9};
    if(c.good && d.good) { c += d; }
    std::cout << "c: " << c << '\n';
    Zlomek e { -13, -666 };
    e.zjednodusenie();
    std::cout << "e: " << e << '\n';
    waitForEnter();
    return 0;   
}

void waitForEnter()
{
    std::cout << "\nPress ENTER to continue...\n";
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

Edit: added an output:
Output:
a: 0; b: -1/-1
Couldn't initialize one of the two zlomek.
c: 11
e: 13/666

Press ENTER to continue...


Note: I chose to turn the failed ‘Zlomek’ into (-1, -1) rather than throwing an exception because I thought it was a clear enough condition: most people would prefer to have a fraction like 1 or 1/1 instead of the equivalent -1/-1. I could be wrong, anyway.
Last edited on
Topic archived. No new replies allowed.