Granny's Programming Pearls
"inside of every large program is a small program struggling to get out"
JavaRanch.com/granny.jsp
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
programming forums Java Mobile Certification Databases Caching Books Engineering Micro Controllers OS Languages Paradigms IDEs Build Tools Frameworks Application Servers Open Source This Site Careers Other Pie Elite all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Jeanne Boyarsky
  • Ron McLeod
  • Paul Clapham
  • Liutauras Vilda
Sheriffs:
  • paul wheaton
  • Rob Spoor
  • Devaka Cooray
Saloon Keepers:
  • Stephan van Hulst
  • Tim Holloway
  • Carey Brown
  • Frits Walraven
  • Tim Moores
Bartenders:
  • Mikalai Zaikin

Code Review Binary ←→ Decimal Conversion.

 
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
The problem is from the book Think like a programmer by V. Anton Spraul.it states:

If you’ve learned about binary numbers and how to convert from decimal
to binary and the reverse, try writing programs to do those conversions with
unlimited length numbers (but you can assume the numbers are small
enough to be stored in a standard C++ int).


C++ is used in the book,that's why he called C++ int.
Here is my code below:

please let me know if there should be some improvement.one thing i would like to ask if you will look in my code my some methods are static along with private.is it okay there.


Kind regards,
Praveen.
 
Marshal
Posts: 79177
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I would suggest that your // comments are too long, and they make the lines difficult to read and I suggest they shou‍ld be /* comments */ instead.
How many bits does a C++ int occupy? Are you sure it is always 32?
Why have you got so many static methods?
Main method: please put a space after the ellipsis operator.
In the ensure length method why are you appending 0s to the StringBuilder?
Why are you using magic numbers, e.g. 32, -2147483648?
What does the flip bit method do? Do you need it?
If you have one class which does the conversions, what is going to happen if you do this?I think there is a design error in your class. Can you see what I am worried about?

There are some confused bits further down. I am not convinced you have completely understood two's complement and its original definition. Remember the leftmost bit is not a sign bit the way the leftmost bit in a floating‑point number is a sign bit. It represents a value. Also, what do you understand by unsigned right‑shift?

I suggest you would do well to explain the algorithm you are using before you try writing any code. It is not difficult, but there are two algorithms, one for decimal to binary and the other for binary to decimal. They are a sort of mirror&#s2011;image of each other.
 
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Likes 1
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
As I was telling someone in a different thread, when an API is awkward, the implementation also tends to be awkward and convoluted.

Campbell hinted that your API is kind of awkward and unnatural and I feel the same way. There are many ways you could go with the design. One is to stick to an OO design. You may also choose to just go more functional and create a utility class with static methods.  A good set of unit tests to explore these different options would help you experiment and see what best fits your intent for the program(s).

I have to run out for a bit. More when I get back.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
If I take the code that's in your main() method and convert it to a unit test, I'd have something like this:

Already, this is raising a lot of questions in my mind about the API. See if you can separate yourself enough from the code and come at it as though you know nothing about this class. What questions come to your mind when you see that test code above? Is there a rational and logical answer you can give to those two questions? Does this test code tell a clear and coherent story about the capabilities of this converter class? Are there any classes in the standard library or a popular third-party library whose API seems like it could be a good model for this class' API?
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
I like that you're posting your work for review because it also gives me a chance to examine my process for reviewing code.  I guess the first question I have in coming to a code review is, "Does this code make any sense?" If the answer is not immediately, "Yes!" then it's time to roll up sleeves and start picking at the nits in the design. Everyone comes to a code review with their own agenda. In my role as a technical leader of the development team, my agenda is to make the work shareable, malleable, and amenable to change by anyone and everyone on the team. My nose is tuned and primed to sniff out code and design smells and the first symptoms I look for are bad names, duplication, and lack of clarity and cohesiveness in the code. I've managed to tick off a bunch of those boxes while reading through your code.

Before I tell you what those smells are, however, it might be a worthwhile exercise for you to try and sniff out and identify some of those smells yourself.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
One thing that has helped me develop a nose for code smells and design smells over the years is the habit of reading lots of other people's code.

I have found that good code has a way of baiting the reader and making them curious. Good code rewards a reader's curiosity by giving them more reasons to dig and search for hidden gems.

I'll often start with the API documentation and look at code samples of how the API is used. Well written documentation and unit tests are very helpful here. Once you start digging, good code and designs make it hard to stop, giving you more and more reasons to dive into the depths of the code and gain insights into the minds of the designers and program authors and the kind of thought process that went into producing such great software.

There are also other times when you only want to know enough to be able to use the API.

The best designs and code are the ones that can satisfy readers on both ends of that spectrum. For the former, a clear and intuitive API is the bait that draws them in and wanting to see more. They want to know what's wonders lie beneath that clean API, like those wide-eyed car aficionados who just have to look under the hood of a new model of their favorite exotic car to admire the engineering that lies under the aesthetics of the exterior. For the latter, it's enough for them to be able to get in the driver's seat and enjoy a quick spin around the block.

 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:I would suggest that your // comments are too long, and they make the lines difficult to read and I suggest they shou‍ld be /* comments */ instead.

Okay i got it.

Campbell Ritchie wrote:How many bits does a C++ int occupy? Are you sure it is always 32?

Actually i am not sure about it in C++.but since i am coding in java i made a convention to deall with java int(32 bits).

Campbell Ritchie wrote:Why have you got so many static methods?

That is the most awkward thing which comes to my mind when,after writing my code,i go through it.actually they seems like a utility method for my class(so i made them static) but what would you advice should i place them in another utility class.

Campbell Ritchie wrote:Main method: please put a space after the ellipsis operator.
In the ensure length method why are you appending 0s to the StringBuilder?

okay i would use a space.actually i realize that is a bad name,i will change it.i want this method to add leading 0's in order to complete the 32 bit length(that's why i had used it,though i will make it a constant).can you advice some better name.

Campbell Ritchie wrote:What does the flip bit method do? Do you need it?

it will flip the argument bit.please look at this method:
if i will not use the flipBit.the statement inside the method either gets too long which seems weird and if i use a variable to store its value inside it then i think it would break the SLAP(single level of abstraction principle.)

Campbell Ritchie wrote:If you have one class which does the conversions, what is going to happen if you do this?I think there is a design error in your class. Can you see what I am worried about?

Okay i got it.it's a bug i will correct it.
I am coming again in some time...
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Campbell wrote:...Remember the leftmost bit is not a sign bit the way the leftmost bit in a floating‑point number is a sign bit. It represents a value. Also, what do you understand by unsigned right‑shift? ...

I don't get your point.java uses a IEEE 754 standard to represent floats but still there the leftmost bit is the sign bit which,In my opinion,does only carry the sign information.have i missed something? please let me know if i have.
unsigned right shift operator(i >>> j) shifts the bits of i by the value(represented by the 5 least significant bits in i for int and by 6 least significant bits for the long).and the unfilled bits were replaced by 0's.

Kind regards,
Praveen.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
@Junilu: I don't know what a unit test is,though i have seen this word several times in the forum,can you please let me know what it is and how to perform it.
For the code you have given,i got what you meant,there is a bug in my code.i think there should be some validation criteria to validate the constructor parameter before going towards any calculation,it just reminds me about bloch advice in his book to validate the parameters before doing any work on it.Though apart from this bug i think i have opted some bad names for my method along with the use of too many static methods,i think it's also breaking the single responsibility principle.I want to stick to OO design so what should i do.

Kind regards,
Praveen.
 
Campbell Ritchie
Marshal
Posts: 79177
377
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

praveen kumaar wrote:. . . .java uses a IEEE 754 standard to represent floats but still there the leftmost bit is the sign bit . . .

But that is what I said. In a floating‑point number the leftmost bit represents the sign and nothing else. But in an integer type the leftmost bit has a value (in two's complement in 32 bit, that value is −2³¹


unsigned right shift operator(i >>> j) shifts the bits of i by the value(represented by the 5 least significant bits in i for int and by 6 least significant bits for the long).and the unfilled bits were replaced by 0's.

Kind regards,
Praveen.

No, it shifts the bits in i by the rightmost 5 bits in j. It is not the same as integer division if the left operand is negative.
You didn't get my point about the StringBuilder. I didn't ask why you were using 0s. You usually want 0s. I was asking why you are appending them.
You have still not explained why you are flipping bits. I am not sure I understand your argument, but it doesn't sound fully thought through. Converting decimal and binary numbers shou‍ld be an easy task and I don't remember flipping bits last time I tried it. You could consider changing the method name to something like addLeading0s, but ensureLength isn't too bad a name. Calling a StringBuilder binaryString is however bad. It looks very strange to set something on a String. Beware of passing StringBuilder references around. You usually would use a StringBuilder as a method‑local variable.

In its simplest form, unit testing means to test each method individually. You pass it normal and abnormal inputs and see how it handles them.
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
After all the suggestions i have tried to stick with the OO design and its principles.I have 3 classes namely BinaryNumber, DecimalNumber, and the converter and one enum type ConversionType.i design the first 2 classes for the binary number and the decimal number represantion.the another class converter will act as a functional object which does the conversion accordingly with the enum constant given to the constructor.i have design the enum type for represnting the conversion i.e., from binary to decimal and decimal to binary.later i can also add some other conversion to it with holding the backward compatibility.I have tried my best to explain the working of everything in the classes through javadoc comments.
Here is my code:
BinaryNumber.java:

DecimalNumber.java:

Enum constant denoting the conversion type;ConversionType.java:

The final class acting as a functional object for doing the conversion;Converter.java:


I will request the members to review my code and suggest anything to make my code more better.

Kind regards,
Praveen.
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Praveen, quickly scanning over this most recent version you posted, it looks pretty decent on the surface. You've obviously made an effort to document your code with JavaDocs and for the most part, it looks like you've done a good job. One little nitpick is that the first sentence in a JavaDoc comment should be in proper sentence case; you missed a few in this respect.

You asked what a unit test is. I was referring to automated unit tests like those you'd run with JUnit.

I've been slowly learning Go and have been watching some talks by Rob Pike, one of Go's creators at Google. In one talk, Rob reminded the audience that "Documentation is for users." He explained how many programmers tend to write program documentation while still thinking about the internal implementation of their programs so their comments tend to be more about implementation rather than intent. Rob said that we should document with the user in mind and tell them more about intent and proper usage.

I think the same idea applies to unit tests. Many programmers write unit tests with a strong focus on internal implementation details. Good tests, however, tell a story. They tell how the program under test is supposed to be used. Good tests are like high-level use cases that show how to prepare the program for use, how to call its API, and what behavior and outcomes to expect. Good high-level tests reveal little to no specifics about implementation. This is how I try to write most of my unit tests.

I'm a big proponent of Test-Driven Development (TDD) and JUnit and these are key to enabling me to write unit tests the way I just described.

If you could write some high-level unit tests that show how this converter of yours is supposed to be used by others, that would be awesome. Then it would be easier to see just how well-designed and well-coded this little utility of yours is.
 
Sheriff
Posts: 7125
184
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Great code!  Well formatted and documented.  I have one nitpick on the documentation too: you should have a space after punctuation.

I think at this level, code should have its own package, not the default package.

--------------

I think the above code would be better as:

--------------------

Negative should be negative, but really, you don't need the boolean variable.

--------------------------------------

I think this would be better:
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator

Knute Snortum wrote:I think this would be better:


Agree with all of Knute's feedback. To add to the one above, yet another refactor wouldn't hurt: Extract Method to replace the formula with an intention revealing method name.
and I might even argue that you should separate i and j:

EDIT: It's easy to imagine someone arguing that the above refactoring is going a step too far, that the exception thrown was enough to explain the formula. There's some merit in that reasoning, however, if you had an optimizing compiler and you declared the outOfBounds() method as private or final, I can easily imagine that an optimizing compiler would see the method calls and decide to inline the one line of code that's in outOfBounds(int, int).  So, if you had an optimizing compiler that inlined the extracted code back anyway, why not just go the extra step to make the code clearer for the reader?
 
Junilu Lacar
Sheriff
Posts: 17644
300
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
Praveen,

Overall, I think you should be happy with the progress that you've made. Keep reading up on the things we've talked about so far, particularly the principles behind making all those improvements. And have a cow for your efforts!  
 
praveen kumaar
Ranch Hand
Posts: 491
23
Eclipse IDE Firefox Browser Spring VI Editor AngularJS Java
  • Mark post as helpful
  • send pies
    Number of slices to send:
    Optional 'thank-you' note:
  • Quote
  • Report post to moderator
@Junilu: sorry for being late, actually i am teaching myself some basics of automated testing with *JUnit Framework*.i have read some articles on it and i like, written somewhere, test a little code a little.till now i had just been coding but from now i will code with confidence(i think what actually the testing provide is the confidence).i will be grateful to you and *knute* for every piece of advide you had given me.thanks a lot again.

Okay,i have also shown my code in other forums and to some of my colleagues.i got several of advices, somebody has adviced me to use char[] instead of stringbuilder(SB) and really by using char[] the readability of my code is enhanced so i have done it.next thing is someone has also adviced me to use some binary arithmetic along with bitwise operators so i have done a change in the *toDecimalInt* and *toBinaryNumber()*.i have also added a static factory as because someone has told me exception throwing in a constructor is basically a bad form because JLS mandates that the Exception be wrapped into an ExceptionInInitializerError so i have done this change.
I have also written a unit test with junit to test my code.
all the things are below:
BinaryNumber.java


BinaryNumberTest.javaAll the test passed.


DecimalNumber.java


DecimalNumberTest.javaAll the test passed.


ConversionType.java


ConversionTypeTest.java→All the test passed


Converter.java


Junilu and knute i will request you to give some suggestions regarding the unit test i have done.as this is my first test code.

reply will be appreciated.

Kind regards,
Praveen.
 
Thanks tiny ad, for helping me escape the terrible comfort of this chair.
a bit of art, as a gift, the permaculture playing cards
https://gardener-gift.com
reply
    Bookmark Topic Watch Topic
  • New Topic