• Post Reply Bookmark Topic Watch Topic
  • New Topic

Refactoring and testing  RSS feed

 
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello All.

I am stuck with a problem with creating unit test for refactoring for this code



Thanks
 
Rancher
Posts: 779
19
C++ Java MySQL Database Netbeans IDE Oracle Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I have no hint on unit testing for you, as this is a fairly straight forward routine, but did you realize that you have 5 exactly the same statements:

That can be replace with:


BTW: where to you calculate "something"?
 
Marshal
Posts: 56610
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
And it can be simplified by removing every instance of != false.
Also don't mess around with Math.random() and arithmetic. Use
1 + random.nextInt(6)
where random is an instance of the Random class.
 
Ranch Hand
Posts: 385
6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Les Morgan wrote:
That can be replace with:



I am confused. Surely the if(m[j]!= false) should be inside the for loop, no?

I mean, in the above code, if j> 0, if m[j] is false, we will still set d[j] ... but the OP doesn't want to set d[j] if m[j] is false. So the OP doesn't want to, for example, set d[4] if m[4] is false.

 
Marshal
Posts: 4053
239
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Well it looks like you've already gotten a whole load of help at another forum where you posted the exact same question.

Was there something you weren't happy with from their help that compelled you to ask again here?

Please read our policy on Cross Posting -> BeForthrightWhenCrossPostingToOtherSites
 
Les Morgan
Rancher
Posts: 779
19
C++ Java MySQL Database Netbeans IDE Oracle Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Indeed! Good catch, it seems your mind is more awake this morning than mine.

Ahmed Bin S wrote:
Les Morgan wrote:
That can be replace with:



I am confused. Surely the if(m[j]!= false) should be inside the for loop, no?

I mean, in the above code, if j> 0, if m[j] is false, we will still set d[j] ... but the OP doesn't want to set d[j] if m[j] is false. So the OP doesn't want to, for example, set d[4] if m[4] is false.

 
Ahmed Bin S
Ranch Hand
Posts: 385
6
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Les Morgan wrote:Indeed! Good catch, it seems your mind is more awake this morning than mine.


Well it is late afternoon here, had it been morning my mind would probably have been as asleep as yours!
 
Les Morgan
Rancher
Posts: 779
19
C++ Java MySQL Database Netbeans IDE Oracle Tomcat Server
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So thanks to Ahmed's keen eyes and other suggestions:

Ahmed Bin S wrote:
Les Morgan wrote:Indeed! Good catch, it seems your mind is more awake this morning than mine.


Well it is late afternoon here, had it been morning my mind would probably have been as asleep as yours!
 
Campbell Ritchie
Marshal
Posts: 56610
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Les Morgan wrote:. . . . . .
No, no, no, no, no.

Apart from the poor formatting, there is a serious logical error in your translation from the original.
 
Mohamed Fareedh
Greenhorn
Posts: 10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Campbell Ritchie wrote:
Les Morgan wrote:. . . . . .
No, no, no, no, no.

Apart from the poor formatting, there is a serious logical error in your translation from the original.


WHat is the problem witth the above suggestion?
 
Campbell Ritchie
Marshal
Posts: 56610
172
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
It is a very little problem to see, and not easy to find, but it is a serious logical change. I shall give you a bit longer to find it.
 
Les Morgan
Rancher
Posts: 779
19
C++ Java MySQL Database Netbeans IDE Oracle Tomcat Server
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My mind is really not up yet: the "!=false" just drops out, so:

I mistakenly put a negation in front of the boolean... no "!" should have been translated over. Good catch Campbell.

Mohamed Fareedh wrote:
Campbell Ritchie wrote:
Les Morgan wrote:. . . . . .
No, no, no, no, no.

Apart from the poor formatting, there is a serious logical error in your translation from the original.


WHat is the problem witth the above suggestion?
 
Campbell Ritchie
Marshal
Posts: 56610
172
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That goes to show how error‑prone things like != false are. If the original code has simply said
if (m[i]) ...
which would have been correct style, there would have been no chance of such errors. Of course using single‑letter names for variables is error‑prone, too.
 
Bartender
Posts: 1840
10
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Just because nobody has mentioned the testing part of it yet...

One of the base things about a test is that it needs to be reproducible. i.e. have the same result every time.
The use of the random numbers in here is going to throw a spanner in the works for that.
As already suggested, the Random class is better than Math.Random because it gives you the nextInt() method, but it would also make your code testable, because the Random class can be told to always generate the same set of random numbers from a given seed - thus making your test repeatable with the same results.

Also with regards to testing, what do you want to assert?
My guess would be that you test only the 'dice' you have selected for 'reroll' get rerolled.
Using objects rather than primitives would potentially help there. If you had a Dice class with a reroll method, you could test for executions of "reroll".
All you can do here I think is maybe set up the array with a list of known values, and make sure that only the ones that you want to get changed.

for instance if you set d to be {0,0,0,0,0} and m to be {true, false, true, false, true}
you would expect to end up with {x, 0, y, 0, z} where x,y and z are your "random" numbers.

 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!