4
\$\begingroup\$

Is there a cleaner way to write this code? It's the start of a hexeditor I'm creating.

I'm aware that the variables aren't descriptive, this is just a test run for when I imbed it into a File Converter program.

def hexeditor
    file = File.read('TESTFILE.txt')
    file2 = File.read("TESTFILE2.txt")
    a = 1
    b = file.unpack('h*')
    c = file.encoding.name
    d = file2.unpack('h*')
    e =file2.encoding.name
    printf("%6s %20s %35s", "LINE:", "HEX:", "ENCODING:\n")
    printf("%4s %25s %11s", a, b, c)
    puts " "
    printf("%4s %25s %11s", a.next, d, e)
    puts " "
end

hexeditor

Currently, the output looks like this:

 LINE:                 HEX:                          ENCODING:
   1 ["4584943502943502140245543545026494c454a0"]    US-ASCII 
   2 ["4584943502943502140245543545026494c454a0"]    US-ASCII
\$\endgroup\$
  • 3
    \$\begingroup\$ Why does it say LINE: when the numbers refer to different files? \$\endgroup\$ – Flambino Dec 7 '15 at 19:50
10
\$\begingroup\$

I've just got a few little tips for this tiny little program:

  1. Use consistent spacing. You mostly have a = b, but in one place you have a =b. Probably just a typo, but worth mentioning.

  2. In Ruby, you have string interpolation, which (in my experience) tends to be preferred to printf.

  3. puts " " is equivalent to puts, at least the way you're using it.

  4. a, b, c, d, and e are uninformative names. Use names that are what they represent -- for example, e would be file2encoding.

  5. Use arrays, instead of multiple variables. Make use of map to turn a list of paths into a list of arrays, which contain the encoding, content, etc. of a file. Maybe make a class to represent the files -- oh, wait, there already is one, called File, which you already use. Then again, it doesn't do much of what you need.

  6. Prefer single-quotes when you don't have interpolation, and double-quotes when you do. Or just always use double-quotes. Don't flip-flop back and forth for no reason.

  7. Your printing-out-bit will not print out each line. It will print out an array, calling it the first line of the first file, which contains every line in the file. That's what the ["..."] around the output means. You want to loop through the array there and, for each one, print out the line number, line, etc. etc.

  8. This isn't Python; you don't need to wrap your main code in a function, then call it at the end. Just write your code. If you want to copy/paste into another file and you're worried about indenting, stop using tools that weren't made for coding and get a decent IDE.

  9. You're using File::read, which isn't a real method. The method you're really using is IO::read. Please try to use the right class when you're calling methods.

  10. The "encoding of the file" that you're outputting is really the encoding of the string that's returned by IO::read. As such, it'll change depending on the system, not depending on the file. This is what the docs are for.

With my advice, here's what your code looks like (untested as of yet, but should work):

files = ["TESTFILE.txt", "TESTFILE2.txt"] # Could also use %w[], but this is safer.
files.each do |file|
  puts "Filename: #{file}"
  puts "Data:"
  IO.foreach(file).with_index do |line, num| # For each line in `file`
    puts "#{num+1}:\t#{line.unpack('h*')}"
  end
end

Much simpler! Notice the .with_index after .foreach -- that's to keep track of the index for you, so you don't have to!

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I know the variables aren't descriptive, this is just a test for when I actually imbed it into my File Converter program. string interpolation, idk if that would help me in this case would it...? Also I was under the impression that puts had to take an argument or would error out? \$\endgroup\$ – 13aal Dec 7 '15 at 19:51
  • 6
    \$\begingroup\$ @Ekult3k "This is a test" is no excuse. If you write good test code, you can just copy/paste, instead of having to refactor it when you move it back. String interpolation is more idiomatic for Ruby, so it's preferable. Did you try puts without an argument? Did you read the docs? Either way, you'd see that without an argument it just outputs a single newline. \$\endgroup\$ – Fund Monica's Lawsuit Dec 7 '15 at 19:53
  • \$\begingroup\$ @QPaysTaxes That's a valid point.. I'll take care of it, thanks for the quick answer. Much appreciated! \$\endgroup\$ – 13aal Dec 7 '15 at 19:56
8
\$\begingroup\$

This has been thoroughly answered already I'd say, but I'll have a go regardless...

You're loading 2 files (I don't know why, but I'll roll with it), and doing the same things with both. That calls for a method, or a loop, or both.

I'm thinking something like this:

%w(TESTFILE.txt TESTFILE2.txt).each.with_index do |path, i|
  data = File.read(path)
  hex = data.unpack('h*')
  encoding = data.encoding.name
  puts "%4s %25s %11s" % [i + 1, hex, encoding]
end

The contents of the loop can be moved to method, if you so choose. If you have to do more stuff than the above, it'd probably be a good idea.

Note, by the way, the syntactatic sugar for printf. As QPaysTaxes says, string interpolation is also an option (perhaps a better one), but format % args is a neat trick and lets you use puts directly instead of calling it afterward, or manually appending "\n" to the stuff you're printing.

If you want to use string interpolation instead, you can use String#rjust and String#ljust to pad the strings with spaces.

|improve this answer|||||
\$\endgroup\$
  • 2
    \$\begingroup\$ Worth pointing out (also, hi again!) that %w() splits things by spaces, so if you have a filename with a space in that list, it's going to give you bugs. \$\endgroup\$ – Fund Monica's Lawsuit Dec 7 '15 at 21:49
  • \$\begingroup\$ Didn't see this answer yesterday. Awesome, thank you for this \$\endgroup\$ – 13aal Dec 8 '15 at 13:05
5
\$\begingroup\$
file = File.read('TESTFILE.txt')
file2 = File.read("TESTFILE2.txt")

It's not clear why you would need 2 files to read, nor what purpose each one is fulfilling: file and file2 are bad names that make the code require a comment to explain what the files are - better names would capture that meaning instead.

a = 1

Again, naming. This should really be line, or currentLine. And it could be defined/assigned closer to usage, too.

b = file.unpack('h*')
c = file.encoding.name
d = file2.unpack('h*')
e =file2.encoding.name

Inconsistent spacing, and names are becoming problematic at that point. If this is the foundation for a hex editor, you're building a very frail foundation.

unpacked1 = file1.unpack('h*')
unpacked2 = file2.unpack('h*')
encoding1 = file1.encoding.name
encoding2 = file2.encoding.name

Now, whenever one feels the need to have a numeric suffix to identifier names, it usually smells of a need for some data structure. I know nothing of , but if it can define a struct or some object (eh, the tag wiki states it's an object-oriented language), you'd have one with some UnpackedHex and EncodingName members, perhaps called HexInfo - and then you could have an array of HexInfo objects to work with.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.