0

I'm trying to store a number in an array of 4 integers. The array is in the class Num. My problem is that when I call getValue, the function returns numbers that aren't correct. I tried go through the program on paper, doing all the calculations in Microsoft's calculator, and the program should give the correct output. I don't even know which function could be problematic since there aren't any errors or warnings, and both worked on paper.

21 in binary:10101

What I'm trying to do:

Input to setValue function: 21

setValue puts the first four bits of 21 (0101) into num[3]. So num[3] is now 0101 in binary. Then it should put the next four bits of 21 into num[2]. The next four bits are 0001 so 0001 goes into num[2] The rest of the bits are 0 so we ignore them. Now num is {0,0,1,5}. getValue first goes to num[3]. There is 5 which is 0101 in binary. So it puts that into the first four bits of return value. It then puts 0001 into the next four bits. The rest of the numbers are 0 so it is supposed to ignore them. Then the output of the function getValue is directly printed out. The actual output is at the bottom.

My code:

#include <iostream>
class Num {
    char len = 4;
    int num[4];
    public:
        void setValue(int);
        int getValue();
};

void Num::setValue(int toSet)
{
    char len1=len-1;
    for (int counter = len1;counter>=0;counter--)
    {
        if(toSet&(0xF<<(len1-counter))!=0)
        {
            num[counter]=(toSet&(0xF<<(len1-counter)))>>len1-counter;
        } else {
            break;
        }
    }
}

int Num::getValue() 
{
    char len1 = len-1;
    int returnValue = 0;
    for(char counter = len1; counter>=0;counter--)
    {
        if (num[counter]!=0) {
            returnValue+=(num[counter]<<(len1-counter));
        } else {
            break;
        }

    }
    return returnValue;
}

int main()
{
    int x=260;
    Num number;
    while (x>0)
    {
        number.setValue(x);
        std::cout<<x<<"Test: "<<number.getValue()<<std::endl;
        x--;
    }



    std::cin>>x;
    return 0;
}

Output:

260Test: -1748023676
259Test: 5
258Test: 5
257Test: 1
256Test: 1
255Test: 225
254Test: 225
253Test: 221
252Test: 221
251Test: 213
250Test: 213
249Test: 209
248Test: 209
247Test: 193
246Test: 193
245Test: 189
244Test: 189
243Test: 181
242Test: 181
241Test: 177
240Test: 177
239Test: 177
238Test: 177
237Test: 173
236Test: 173
235Test: 165
234Test: 165
233Test: 161
232Test: 161
231Test: 145
230Test: 145
229Test: 141
228Test: 141
227Test: 133
226Test: 133
225Test: 1
224Test: 1
223Test: 161
222Test: 161
221Test: 157
220Test: 157
219Test: 149
218Test: 149
217Test: 145
216Test: 145
215Test: 129
214Test: 129
213Test: 125
212Test: 125
211Test: 117
210Test: 117
209Test: 113
208Test: 113
207Test: 113
206Test: 113
205Test: 109
204Test: 109
203Test: 101
202Test: 101
201Test: 97
200Test: 97
199Test: 81
198Test: 81
197Test: 77
196Test: 77
195Test: 5
194Test: 5
193Test: 1
192Test: 1
191Test: 161
190Test: 161
189Test: 157
188Test: 157
187Test: 149
186Test: 149
185Test: 145
184Test: 145
183Test: 129
182Test: 129
181Test: 125
180Test: 125
179Test: 117
178Test: 117
177Test: 113
176Test: 113
175Test: 113
174Test: 113
173Test: 109
172Test: 109
171Test: 101
170Test: 101
169Test: 97
168Test: 97
167Test: 81
166Test: 81
165Test: 77
164Test: 77
163Test: 69
162Test: 69
161Test: 1
160Test: 1
159Test: 97
158Test: 97
157Test: 93
156Test: 93
155Test: 85
154Test: 85
153Test: 81
152Test: 81
151Test: 65
150Test: 65
149Test: 61
148Test: 61
147Test: 53
146Test: 53
145Test: 49
144Test: 49
143Test: 49
142Test: 49
141Test: 45
140Test: 45
139Test: 37
138Test: 37
137Test: 33
136Test: 33
135Test: 17
134Test: 17
133Test: 13
132Test: 13
131Test: 5
130Test: 5
129Test: 1
128Test: 1
127Test: 225
126Test: 225
125Test: 221
124Test: 221
123Test: 213
122Test: 213
121Test: 209
120Test: 209
119Test: 193
118Test: 193
117Test: 189
116Test: 189
115Test: 181
114Test: 181
113Test: 177
112Test: 177
111Test: 177
110Test: 177
109Test: 173
108Test: 173
107Test: 165
106Test: 165
105Test: 161
104Test: 161
103Test: 145
102Test: 145
101Test: 141
100Test: 141
99Test: 133
98Test: 133
97Test: 1
96Test: 1
95Test: 161
94Test: 161
93Test: 157
92Test: 157
91Test: 149
90Test: 149
89Test: 145
88Test: 145
87Test: 129
86Test: 129
85Test: 125
84Test: 125
83Test: 117
82Test: 117
81Test: 113
80Test: 113
79Test: 113
78Test: 113
77Test: 109
76Test: 109
75Test: 101
74Test: 101
73Test: 97
72Test: 97
71Test: 81
70Test: 81
69Test: 77
68Test: 77
67Test: 5
66Test: 5
65Test: 1
64Test: 1
63Test: 161
62Test: 161
61Test: 157
60Test: 157
59Test: 149
58Test: 149
57Test: 145
56Test: 145
55Test: 129
54Test: 129
53Test: 125
52Test: 125
51Test: 117
50Test: 117
49Test: 113
48Test: 113
47Test: 113
46Test: 113
45Test: 109
44Test: 109
43Test: 101
42Test: 101
41Test: 97
40Test: 97
39Test: 81
38Test: 81
37Test: 77
36Test: 77
35Test: 69
34Test: 69
33Test: 1
32Test: 1
31Test: 97
30Test: 97
29Test: 93
28Test: 93
27Test: 85
26Test: 85
25Test: 81
24Test: 81
23Test: 65
22Test: 65
21Test: 61
20Test: 61
19Test: 53
18Test: 53
17Test: 49
16Test: 49
15Test: 49
14Test: 49
13Test: 45
12Test: 45
11Test: 37
10Test: 37
9Test: 33
8Test: 33
7Test: 17
6Test: 17
5Test: 13
4Test: 13
3Test: 5
2Test: 5
1Test: 1

I compiled this with g++ 6.3.0 with the command g++ a.cpp -o a.exe

2
  • You shift it while it is not necessary to shift it. You have an array of 4 ints to store 4 ints. Do you know what shift is anyway? Your code looks like it is very overcomplicated. i cannot figure out the actual purpose.
    – Codebeat
    Sep 23, 2018 at 1:06
  • When you see values like in your 260Test, you should ask yourself if you've failed to initialize a variable before use. Hint, the answer is yes. Sep 23, 2018 at 1:07

2 Answers 2

0

When compiling with -Wall, there are a number of warnings:

orig.cpp: In member function ‘void Num::setValue(int)’:
orig.cpp:15:39: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]
         if(toSet&(0xF<<(len1-counter))!=0)
                  ~~~~~~~~~~~~~~~~~~~~~^~~
orig.cpp:17:61: warning: suggest parentheses around ‘-’ inside ‘>>’ [-Wparentheses]
             num[counter]=(toSet&(0xF<<(len1-counter)))>>len1-counter;
                                                         ~~~~^~~~~~~~
orig.cpp: In member function ‘int Num::getValue()’:
orig.cpp:30:24: warning: array subscript has type ‘char’ [-Wchar-subscripts]
         if (num[counter]!=0) {
                        ^
orig.cpp:31:38: warning: array subscript has type ‘char’ [-Wchar-subscripts]
             returnValue+=(num[counter]<<(len1-counter));
                                      ^

If you were to print the values of num before changing them, you'd see that some might be non-zero (i.e. they are uninitialized), which causes undefined behavior and probably breaks your for loops in getValue and setValue.

So change:

int num[4];

Into:

int num[4] = { 0 };

Here's a cleaned up version with the warnings fixed:

#include <iostream>
class Num {
    int len = 4;
    int num[4] = { 0 };
    public:
        void setValue(int);
        int getValue();
        void showval();
};

void Num::setValue(int toSet)
{
    int len1=len-1;
    for (int counter = len1;counter>=0;counter--)
    {
        if ((toSet & (0xF << (len1-counter))) != 0)
        {
            num[counter] = (toSet & (0xF << (len1-counter))) >> (len1-counter);
        } else {
            break;
        }
    }
}

int Num::getValue()
{
    int len1 = len-1;
    int returnValue = 0;
    for(int counter = len1; counter>=0;counter--)
    {
        if (num[counter]!=0) {
            returnValue+=(num[counter]<<(len1-counter));
        } else {
            break;
        }

    }
    return returnValue;
}

void Num::showval()
{

    for (int i = 0;  i < len;  ++i)
        std::cout << i << ": show: " << num[i] << "\n";

#if 0
    for (int i = 0;  i < len;  ++i)
        num[i] = 0;
#endif
}

int main()
{
    int x=260;
    Num number;

    number.showval();

    while (x>0)
    {
        number.setValue(x);
        std::cout << x << " Test: " << number.getValue() << std::endl;
        x--;
    }

    std::cin>>x;
    return 0;
}
0

To break a number into nibbles, the shift counts should be multiples of 4. Otherwise slices of 4 bits are extracted that don't line up.

00010101 (21)
    ^^^^ first nibble
^^^^ second nibble

The second nibble is displaced by 4 bits so it needs to be shifted right by 4, not by 1.

You could multiply your shift counts by 4, but there is an easier way: only ever shift by 4. For example:

for (int i = len - 1; i >= 0; i--) {
    num[i] = toSet & 0xF;
    toSet >>= 4;
}

Then every iteration extracts the lowest nibble in toSet, and shifts toSet over so that the next nibble becomes the lowest nibble. I didn't put in a break and there should not be one. It definitely shouldn't be the kind of break that you had, which stops the loop also whenever a number has a zero in the middle of it (for example in 0x101 the middle 0 causes the loop to stop). The loop also should not stop when the entire rest of the number is zero, since that leaves junk in the other entries of num.

It's more common to store the lowest nibble in the 0th element and so on (then you don't have to deal with all the "reverse logic" with down-counting loops and subtracting things from the length) but that's up to you.

Extracting the value can be done symmetrically, building up the result while shifting it, instead of shifting every piece into its final place immediately. Or just multiply (len1-counter) by 4. While extracting the value, you also cannot stop when num[i] is zero, since that does not prove that the rest of the number is zero too.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.