This week's book giveaway is in the Cloud/Virtualization forum.
We're giving away four copies of Building Blockchain Apps and have Michael Yuan on-line!
See this thread for details.
Win a copy of Building Blockchain Apps this week in the Cloud/Virtualization forum!
  • 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 all forums
this forum made possible by our volunteer staff, including ...
Marshals:
  • Campbell Ritchie
  • Paul Clapham
  • Liutauras Vilda
  • Knute Snortum
  • Bear Bibeault
Sheriffs:
  • Devaka Cooray
  • Jeanne Boyarsky
  • Junilu Lacar
Saloon Keepers:
  • Ron McLeod
  • Stephan van Hulst
  • Tim Moores
  • Carey Brown
  • salvin francis
Bartenders:
  • Tim Holloway
  • Piet Souris
  • Frits Walraven

Consider my new program: AsciiTransformer

 
Ranch Foreman
Posts: 74
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hello!
I wrote a new program to test my skills. I need a feedback.

asciiTransformerExample.jpg
[Thumbnail for asciiTransformerExample.jpg]
 
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You are doing well to separate the work into multiple methods and run everything from the startTransformer() method. But both the class and all its non‑private members need proper /** documentation comments. */
Line 10 looks wrong. You shouldn't use magic numbers like that, least of all when you could use char literals instead.
Lines 12, 15, and 20. Using int return values looks like an escape from C programming. You have a boolean type: use it. Or write a loop which tests the actual value as it goes.
Why are the numbers different in lines 10 and 32? [addition]Have you changed line 32?[addition]
Line 14. Isn't there a method to append tne int directly as a code point and obviate the (cast)?
You don't need continue; because you have more by luck than good management written a loop whose entire logic is encapsulated in its heading. You only need a semicolon. That often produces illegible code, so it is probably best not to look on that as at all elegant.
Don't use \n, which is probably the wrong line end. Use this, or printf() and the %n tag, or a few other options, instead.
If you intend to run the reading from the startTransformer() method, why don't all the other methods have private access. Since you haven't written a package name, you won't notice a difference between package‑private (=default) and public access. I don't think methods like printThisText() need to be accessed from outside this class.
Your formatting is inconsistent; you have spaces where they shouldn't be and vice versa.
Why haven't you written a constructor and a toString() method?
 
Sheriff
Posts: 15043
252
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Not bad. You've broken the program down into small chunks which is good. However, the program doesn't match the sample image: the prompts are different and the output is slightly different. Also, the prompt in the code says "65-122"  but the code accepts 32 to 122.

The continue on line 21 is not necessary. You could delete it and just leave the semicolon and the code would work the same way but it would look a little weird.


 
Campbell Ritchie
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Also, that code is designed with proper OO principles in mind.
 
Junilu Lacar
Sheriff
Posts: 15043
252
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I think as an OO program, it's still debatable. Consider the state of such an object. If you had multiple instances, each would have their own Scanner associated with System.in. Is that a good thing? Could you run a program that uses multiple instances? Would you need multiple instances? What about sharing an instance across multiple threads? What about inheritance? Is this class extensible? There's quite a bit more to OO than just breaking down behavior into small chunks.

The naming also suggests that the thinking around design was focused on behavior from an "insider" perspective. For example, "print this text" does not make much sense if you're on the outside looking in. And the name startTransformer is redundant. Let's look at how this program might be run:

Line 2 above is like having a Car object and a startCar() method, so you'd have to write car.startCar() instead of just car.start()

All these are subtle though. As a start, the code is better than most beginner's code we see around here.
 
Bartender
Posts: 3776
154
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
hi Mike,

a preference thing and an omission.

1) I would like to see the method 'scanOneSymbol' return a boolean. In the method 'scanAllSymbols' you can then simply say: while(scanOneSymbol());

2) what do you notice if you have this main():
 
Campbell Ritchie
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
No, it isn't thread‑safe. Not even close. Agree the Scanner instance could be static, and so could the instructions() method. But I don't think that makes it non‑OO code. The important state is stored in the string builder object. It is possible to store that state simply by maintaining a reference to an AsciiTransformer object, as you showed, and even run its methods again:-MS isn't a native English speaker. I agree names like printThisText() aren't absolutely 101% brilliant, but I think that method ought to be private anyway (I think you and I are in agreement about that). I think it is a far closer approximation to good OO than much of what we see, and I hope I can persuade you to agree. Don't get me started about instructors who don't know how to write OO themselves. Or Junilu will join in
 
Campbell Ritchie
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Piet Souris wrote:. . . what do you notice if you have this main(): . . .

Damn! Piet beat me to it showing you can call the method twice. Well done, Piet.
 
Piet Souris
Bartender
Posts: 3776
154
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Beating the Great Campbell... YEAH!!!      
 
Mike Savvy
Ranch Foreman
Posts: 74
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Okay, I changed my code, but did not understand all advises.




P.S. I added a space character later to the output.
Campbell Ritchie,
(1) why I need a constructor here, if Java makes it for me?
(2) "method to append tne int directly as a code point and obviate the (cast)?" -> Which method do you mean?
(3) Do not really understand what is the difference between \n and %n.
(4) "Your formatting is inconsistent;" -> Can you give me an example?
newEncoder.jpg
[Thumbnail for newEncoder.jpg]
 
Junilu Lacar
Sheriff
Posts: 15043
252
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
The new changes didn't necessarily make things better. The toString() method in particularly is not good because it's destructive (it "cleans" the object every time it is called). Anyone calling toString() is in for a rude surprise when they find that they had somehow clobbered everything. You don't get that kind of behavior with any other toString() implementation. The toString() method should be considered idempotent, meaning the result of calling it multiple times should be the same as long as the object state doesn't change. In other words, an idempotent call should not change the state of the object.

The biggest problem with this code that violates OO principles is the mixing of its "transforming" responsibility and its input/output concerns. This mixing of concerns makes the code more difficult to test and it binds/couples it tightly to a particular input/output method. It violates the Single Responsibility Principle that states that there should be one and only one reason to change a class. As written, there can be multiple unrelated reasons for changing this class: Change the prompts, change the range of characters allowed, change the flow, change the instructions. Really, the only responsibility something called AsciiTransformer should have is what is directly related to the job of, well, transforming ASCII.
 
Junilu Lacar
Sheriff
Posts: 15043
252
Mac Android IntelliJ IDE Eclipse IDE Spring Debian Java Ubuntu Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

MS is not a native English speaker



Reminds me of a sign for a lunch bento I saw in Japan last year. The cute thing was that they had crossed out the "R" in runch and written above it "Sorry for making mistake"
 
Mike Savvy
Ranch Foreman
Posts: 74
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
sorry  
 
Marshal
Posts: 6869
182
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Mike Savvy wrote:(3) Do not really understand what is the difference between \n and %n.


"\n" is an escape code for line feed (ascii 10), sometimes called a new line.  It is used in Strings.  Different OSs use different end-of-line escape characters.  "\n" doesn't account for this.

"%n" is a token for an OS-aware end-of-line character.  It is used in printf and String.format.  In a way, it says, "At this point, insert the correct end-of-text character(s) for this OS."
 
Knute Snortum
Marshal
Posts: 6869
182
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Mike Savvy wrote:(2) "method to append tne int directly as a code point and obviate the (cast)?" -> Which method do you mean?


I think Campbell is talking about Scanner#appendCodePoint(int), so instead of writing
you could write
 
Campbell Ritchie
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Knute Snortum wrote:. . . Scanner#appendCodePoint(int) . . .

StringBuilder#appendCodePoint(int) more likely
 
Campbell Ritchie
Marshal
Posts: 68110
258
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am still not happy about the scan one symbol method. I am not convinced that follows single resposibility, or whatever you call the one action per method principle. It verifies the input, it adds it to the String Builder, and (in the earlier version) prints, “Finished!” I think I would change it to something like this:-Alternative using a for loop, which has the advantage that I can declare n inside the loop.
 
Knute Snortum
Marshal
Posts: 6869
182
Eclipse IDE Postgres Database VI Editor Chrome Java Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

Campbell Ritchie wrote:

Knute Snortum wrote:. . . Scanner#appendCodePoint(int) . . .

StringBuilder#appendCodePoint(int) more likely


Yes, of course.  Sorry for the confusion.
 
this llama doesn't want your drama, he just wants this tiny ad for his mama
Java file APIs (DOC, XLS, PDF, and many more)
https://products.aspose.com/total/java
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!