Review Latest Code

[cpilkington78]

Please review the latest ASCII Translator code and make sure I'm on the right track. Working on how to read the file and strip off the first character after reading the first character (b, o, h) so that the program knows how to create the message. Will update code after I have figured this out.

[cchampio]

Ok I will look this weekend.

[cchampio]

Sounds like you are on the right track. Like you said, you will want to read the first character into a variable. Then you will read the remainder of the file into another variable. Then you will translate the message based on the type (hex, bin or oct) identified by the first character in the file. Don't forget to do some basic validation of the data read from the file to insure the validity of the data before processing it.

I will now look at the code and then report my findings here.

[cchampio]

Looked at the code:

• I recommend go ahead and extract a few methods of code that are similar.

e.g. you are doing a lot of two writelines, then a thread.sleep, then a exit(0). Consider extracting those 4 lines into a method and passing in the two strings.

• It does look like you are making good progress. You have successfully read the command line args /f and then the filename. You have read the file into a string. This is fine and will work. You will need to find a way to parse this string because you want to get just the first letter into a variable (this identifies the type) and then just the message into another variable. E.g:

//right now you have string strDataReadFromFile = "b10010110"; //you want string Type = "b"; string Data = "10010110"; //this will make the data easier to process.

One way you can get to this is to use the Substring method on the strDataReadFromFile variable.

Another option is to consider modifying your read file code a little bit such that you read only the first character into a variable, then you read the rest of the file. Now you immediately have data in separate variables and there is no need to call Substring. If/when you decide to try this (I recommend you do go to this, but not necessarily right now, it can be an optimization later if you want), look at the StreamReader.Peek and the StreamReader.Read methods. You would still need StreamReader.ReadLine as well. The basic pseudo-code would look like this:

reader = create stream reader IF reader.Peek() >= 0 //is there data in the file?
type = reader.read //read the first character data = reader.readline //read the remainder of the file. END IF

This is not complete because it doesn't handle if the message is broken into multiple lines, but it is the basics of what the code will look like.

[cpilkington78]

Ok. I have gotten the file reading code finished except for the reader.Peek() part. I will implement that tomorrow. I also will extract out some more methods tomorrow.

Please review code and comment.

[cpilkington78]

Also, I uploaded my test files to GitHub so you can download them to test with.

[cchampio]

cool i will have a look today.

On Fri, Jul 3, 2015 at 11:26 PM, Chris Pilkington notifications@github.com wrote:

Also, I uploaded my test files to GitHub so you can download them to test with.

— Reply to this email directly or view it on GitHub https://github.com/cpilkington78/ASCIITranslator/issues/5#issuecomment-118460938 .

Chris Champion

[cpilkington78]

I eliminated the variable "temp" in the ReadFile() method. I will look at extracting out some methods now before adding error checking. Once I have all of the error checking in place, I think I am ready to refactor into classes and more methods to make this project OO instead of Procedural. Let me know if you have any other suggestions.

[cchampio]

Code in looking good. You updated the ReadFile as I suggested and it looks good expect there is no Peek which may cause a bug but that is ok we can fix those minor details after we get this thing using classes. I recommend go ahead and start thinking of what classes you want to extract.

I think I originally send you an email of some class hierarchy that you may want to consider. If you need that again let me know. Give it a shot but if you get stuck let me know. This is your first time to do a design with classes so don't worry too much if you get stuck, that is to be expected. The the end I will show you have I would design it too.

[cpilkington78]

I added the Peek to the ReadFile() method and error handling for a file that is blank. I also moved my test files into a folder inside the project itself so that they are not mixed in with the program files at the master branch level.

I will now work on factoring out methods and classes. Please review edited code and reply with comments/suggestions.

[cpilkington78]

Ok. The functionality of the program is complete. It works! And, it passes all of the tests that I can think of to try! Now, I'm working on method refactoring. Then, I will work on adding the classes and making this program Object Oriented. Please review my latest code and reply with feedback.

However, I DO have one question. Should I make an ErrorCode class and add the enum as well as the GetErrorMessage() method to that class? Does the enum have to be in the Program class? Or, can the enum be in ANY class? Or, is this even a class I would need/want to create?

Please review latest code and reply with comments/suggestions.

[cchampio]

The enum can be inside any class. It doesn't even have to be in a class, you could just put it in a namespace if you wanted to.

An ErrorCode class sounds reasonable. Maybe it contains the enum of ErrorCodes and then the method that returns a friendly error message for each error code. I suggest try it that way and see how you like it; if you think of a better way later you can always change it around.

[cchampio]

I will take a look at the code tomorrow

[cpilkington78]

Ok. I have finished my first phase of re-factoring. Next, I think I need to try to get the validate methods into a single method and maybe work on the validate class. Trying to eliminate all of those Thread.Sleep and Environment.Exit lines of code. May need help with the validate class though.

Please review my latest code and let me know what you think so far.

Thanks!

[cpilkington78]

Also, not sure my ErrorCodes class is correct. I think my enum should be private, but I couldn't figure out how to incorporate the switch into a method to retrieve the enum and make ONLY the GetErrorMessage method public. Please hint and elaborate on how to make that class better.

Thanks!

[cchampio]

Your enum is fine. It needs to be public so that other classes such as the main program can see it. The rest of your program will only deal with ErrorCodes enum and it will request the error message using GetErrorMessage and that error code as needed.

I recommend rename the enum from errorCodes to just Codes since it is already inside the context of the ErrorCodes class.

Other than that the ErrorCodes class is fine for now. We can revisit it later if we think of something that will better suit our needs.

You mentioned a Validate class. A validate class will work fine and you're welcome to use it. However, let me suggest maybe another design and you can choose.

Notice how your Validate methods all say "ValidateBinData" or "ValidateOctData" or "ValidateHexData". Also notice how each of those methods do similar work. They each do a check (e.g. BinaryCheck) that looks at the range of each character. In addition, they each do another check to make sure the length of the message is acceptable (e.g. it must be length 8 for binary).

Recall that we are probably going to want to have a BinaryTranslator, HexTranslator and a OctalTranslator (and a abstract base class Translator too). Those classes will be responsible for translating the message. Consider if the Translator class were also responsible for validating the message before translating it. Then each translator class could have a Validate method. The Validate method will be in the context of the specific translator class (e.g. BinaryTranslator) so there is no longer a need to name it BinValidate or HexValidate because of the context it is in. Since we noticed that the validate methods all do similar work, it might be possible that we could put some of that validation work in the base class and have each specific translator do the unique work.

Just some thoughts to consider. Let me know if you have questions.

Keep in mind that software design is an evolving thing. You know the least you will ever know about the software and the design up front at the beginning, so it is natural that you will be able to improve it the further you go along. Expect it to change as you learn more about what classes and methods you really need.

[cpilkington78]

Ok. I changed the enum name from errorCodes to just Codes. I also created a BinTranslator class. Please review the new code and let me know if I am on the right track for the Translator classes. Also, I forgot the structure of the classes, i.e. what should be at the top, middle, bottom, etc. Please elaborate.

I will wait before continuing on with the Translator classes until I hear from you. Let me know if I'm on the right track and please provide hints/comments.

Thanks!

[cchampio]

Answered the order of classes in a separate code review report.

I quickly looked at the BinTranslator class. I will look again tonight. But I noticed that all the methods in it are static. For this class we will not want any static methods or static properties. All methods in the Translator classes will be instance methods. Other than that I think it looks like a good start.

[cpilkington78]

Ok. I have created the OctTranslator and HexTranslator classes. All of the translator classes still have static methods. I will work on making those instance methods next. Just getting the classes working and all the same so it will be easier to edit.

Also, I am thing about a Validate class that will validate the user input for the type of translation and the file path. What are your thoughts on this?

Let me know if you see anything else you would fix or change before continuing with refactoring into classes.

Thanks!

[cpilkington78]

Btw, I am THINKING about a Validate class, not THING about! LOL!