Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"AboutBitWiseOperations" a few things to look at #349

Closed
ghost opened this issue Jan 20, 2020 · 3 comments · Fixed by #350
Closed

"AboutBitWiseOperations" a few things to look at #349

ghost opened this issue Jan 20, 2020 · 3 comments · Fixed by #350
Labels
Category-Koans Invoking the Great Doubt Issue-Bug 🐛 Something's wrong! Issue-Discussion Let's talk about it!

Comments

@ghost
Copy link

ghost commented Jan 20, 2020

Issue 1

Original Code at Line 52
52 if ($To -ne 'String') {

Modified Code at Line 52
52 if ($To -ne 'ASCIIString') {

Conversions as specified in the rest of the script have lots of problems

Issue 2

Original Code starting a line 204
204 $Value = '00000011' | ConvertFrom-Binary
205 $Result = '________' | ConvertFrom-Binary
206
207 -bnot $Value | Should -Be $Result

Modified Code at line 205
205 $Result = ' ________ ' | ConvertFrom-Binary -To ' ________ '

Since by default $Value is a signed value the result of “bnot” makes it negative, to deal with this you have to select the correct conversion type to work with the $Result value.

Since you deal with signed values later in the koan, this may be a bit early to be discovering this scenario.

Issue 3

Original Code at line 351
351 $Value.Sum | Should -Be 1652126821

Modified Code at line 351
351 $Value.Sum | Should -Be 1075843080

No sure if this was just bad math on someone's part or not.

Issue 4

Original Code at line 394
394 '1000000' | ConvertFrom-Binary -To SByte

Modified Code at line 394
394 '10000000' | ConvertFrom-Binary -To SByte

You are missing a “0”.

Issue 5

Original Code at line 463 - 465
463 $Date = [DateTime]::FromFileTime(($BinaryValue | ConvertFrom-Binary -To Int64))
465 $Date | Should -Be '01 January 1601 00:02:45'

Modified Code at line 463 - 467
463 $Date = [DateTime]::FromFileTime(($BinaryValue | ConvertFrom-Binary -To Int32))
465 $Date.DateTime | Should -Be 'Sunday, 31 December, 1600 6:02:45 PM'
466 $Date.Ticks | Should -Be 504911017652126821
467 $Date | Should -Be ([DateTime]'1600-12-31T18:02:45.2126821-06:00')

There are a couple of issues here

  1. At line 463 there is no option to convert to Int64 and when added a conversion to Int64 doesn’t work.
  2. The Binary object and the date being tested for don’t match
  3. The format of the value in the Should -Be doesn’t match the format when compared strictly to the $Date DateTime object.
    So there are a couple of different ways to play with this
  4. Change the conversion on line 463 from Int64 to Int32
  5. Test against specific properties of the $Date DateTime object
    a. The string representation: $Date:DateTime
    b. Maybe look at the number of ticks: $Date.Ticks
  6. Convert a string representation to a DateTime object for comparison.

I may have completely missed the boat here, if so let me know.

Issue 6

Original Code at line 510
510 [BitConverter]::ToInt32($Bytes, 0) | Should -Be 1702132066

Modified Code to replace line 510
If([BitConverter]::IsLittleEndian)
{
[BitConverter]::ToInt32($Bytes, 0) | Should -Be 1652126821
}
else
{
[BitConverter]::ToInt32($Bytes, 0) | Should -Be 1702132066
}

Because surprise, surprise, my system operates in Little Endian, so you have to check before you do the Should -Be Test, and others may experience a similar problem.

@ghost ghost added Category-Koans Invoking the Great Doubt Issue-Discussion Let's talk about it! labels Jan 20, 2020
@vexx32 vexx32 added the Issue-Bug 🐛 Something's wrong! label Jan 21, 2020
@vexx32
Copy link
Owner

vexx32 commented Jan 21, 2020

This is a really thorough write up, thank you! 😊 💖

I never expected to run into endianness issues here tbh, very interesting to know that can be an issue! The rest of these seem fairly sensible alterations on the surface, but I'd need to take a detailed look at it in context, it's been a little bit since I read through this file. 🙂

@indented-automation since you wrote these ones, do these all look like sensible changes to you? If you're interested in making some / all of the suggested changes I'm happy to let you have at it. If not, I'm more than happy to take a closer look myself! 😊 🌸

@indented-automation
Copy link
Contributor

On it :)

@indented-automation
Copy link
Contributor

Issue 1

Thanks, too much fiddling again!

Issue 2

It's negative because it returns Int32 which is slightly frustrating. Added a handler for it and a new comment to explain why.

Issue 3

I probably fiddled with the example one time too many. Let's go with bad math :) Thanks!

Issue 4

Thanks! Fixed.

Issue 5

Added the missing option for To. Fixed the date comparison.

Issue 6

Most / all modern systems will be little endian (for byte ordering). You have to go back to some really old IBM / Sparc type systems to get Big Endian. Too many bugs in that particular example. It has been corrected now.

PR incoming.

@vexx32 vexx32 added this to To do in Koan Topic Tracking via automation Jan 22, 2020
Koan Topic Tracking automation moved this from To do to Done Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category-Koans Invoking the Great Doubt Issue-Bug 🐛 Something's wrong! Issue-Discussion Let's talk about it!
Projects
Development

Successfully merging a pull request may close this issue.

2 participants