ISBN validator

[GnuVince]

I have written a ISBN number verificator. Can anyone tell me if I could improve this code somehow?

You can find information about validating ISBN numbers at: http://acm.uva.es/p/v3/333.html


#!/usr/bin/env ruby

class ISBN
def initialize(code)
@code = code
end

def calc(lst)
ans = []
ans[0] = lst[0]
for i in 1...lst.length
ans[i] = ans[i-1] + lst[i]
end
ans
end

def validChars
stripCode = @code.tr("-", "")
if stripCode =~ /^[0-9]{9}[0-9X]$/i
true
else
false
end
end

def validate
if validChars then
stripCode = @code.tr("-", "")
@s0 = []
stripCode.each_byte { |c|
@s0 << c.chr
}
@s0[-1] = 10 if @s0[-1] =~ /x/i
@s0.map! { |x| x.to_i }
else
raise "Invalid ISBN"
end

@s1 = calc(@s0)
@s2 = calc(@s1)

raise "Invalid ISBN" if @s2[-1] % 11 != 0
end
end

while true
print "> "
code = gets.strip
begin
isbn = ISBN.new(code)
isbn.validate
puts "#{code}: Valid ISBN"
rescue Exception => e
puts e
end
end

[jemfinch]

As far as your code goes, I've got mostly interface comments to make.

You don't really need a class to do this -- it's more of a function thing. There's no need to maintain any sort of state, so a function that takes a string returns a bool saying whether the string is a valid ISBN or not is probably a better way to do it.

Exceptions should really only be used for exceptional, unexpected events. What you're writing is a validator for ISBN codes -- certainly it's expected that some might fail (otherwise you wouldn't be writing a validator.) Instead of raising an exception on an invalid ISBN, you should just make "validate" return a bool that says whether or not the ISBN is valid. Then you just use an if/then/else statement (instead of a begin/rescue statement) to print your results.

The "calc" function might do with a better, more descriptive name.

Here's an SML version I whipped up in a very functional style:


val validISBN = let
val validChar = Char.contains "0123456789- "
in
String2.foldl (fn (c, acc) => acc andalso validChar c) true
end

val digitsToInts = List.map (fn #"X" => 10 | c => Char.ord c - 48) o explode

val filterISBN = String2.filter (fn c => Char.isDigit c orelse c = #"X")

fun partialSum [] = []
| partialSum (hd :: tl) =
List.foldl (fn (i, l as hd :: tl) => i + hd :: l) [hd] tl

fun goodISBN s =
validISBN s andalso
0 = (List.hd o partialSum o partialSum o digitsToInts o filterISBN) s mod 11


Vale, amice!

Jeremy

[Halide]

wow, jemfinch is an elite coder...

too bad he got owned by smilies :)

[inkedmn]

here's mine in python:

# the isbn verifier
# coded by inkedmn

import sys

def verifyISBN(isbn):
sum1 = 0
sum2 = 0
nums = list(isbn)
for num in nums:
if num != '-':
if num == 'X':
num = 10
try:
sum1 += int(num)
except:
print "Invalid ISBN"
sys.exit(1)
sum2 += sum1
if sum2 % 11 == 0:
print "Valid ISBN"
else:
print "Invalid ISBN"

if __name__ == '__main__':
if len(sys.argv) == 2:
verifyISBN(sys.argv[1])
else:
print "Usage: %s <ISBN>" % sys.argv[0]


:)

[GnuVince]

Thanks to the much more clever algorithm inkedmn has posted (good job buddy), I have changed my program a bit:


#!/usr/bin/env ruby

class ISBN
def initialize(code)
@code = code
end

def check
s1 = 0
s2 = 0
nums = @code.strip.delete(" -").split("")

for n in nums
return false unless n =~ /[\dx]/i
n = 10 if n =~ /x/i
s1 += n.to_i
s2 += s1
end

(s2 % 11).zero?
end
end

if ARGV.length == 1
isbn = ISBN.new(ARGV[0])
if isbn.check then
puts "Valid ISBN"
else
puts "Invalid ISBN"
end
else
puts "Usage: #$0 <isbn>"
end


Yay for open source, code sharing and programming communities!

[Halide]

pretty code :)

[GnuVince]

Merci

[jemfinch]

Originally posted by GnuVince

for n in nums
return false unless n =~ /[\dx]/i
n = 10 if n =~ /x/i
s1 += n.to_i
s2 += s1
end



You should do the regexp test outside the loop:


return false unless n =~ /[\dX]+/
for n in nums
n = 10 if n == 'X'
...


Also note that ISBNs must use a capital X. A lowercase X isn't valid. So switching the "if n =~ /x/i" to "if n == 'X' " (assuming that's valid Ruby syntax) is a more efficient way to do it.

[GnuVince]

jemfinch: Thanks, I wasn't sure if only capital 'X' was accepted. I sure appreciate. Thanks for the regexp tips too.

[inkedmn]

ok, so i was bored...

here it is in java:



// the ISBN verifier
// coded by inkedmn

public class ValidateISBN {

static int sum1 = 0;
static int sum2 = 0;

public static void getSums(String strISBN) {
char[] nums = strISBN.toCharArray();
for(int i = 0; i < nums.length; i++) {
if (nums[i] == 'X') {
sum1 += 10;
sum2 += sum1;
continue;
} else if (nums[i] == '-') {
continue;
} else {
try {
String numChar = Character.toString(nums[i]);
int num = Integer.parseInt(numChar);
sum1 += num;
sum2 += sum1;
} catch (Exception e) {
System.out.println("Invalid ISBN");
System.exit(1);
}
}
}
}

public static boolean isValid() {
if (sum2 % 11 == 0) {
return true;
} else {
return false;
}
}

public static void main(String[] args) {
if (args.length != 1) {
System.out.println("Usage: ValidateISBN <ISBN NUMBER>");
System.exit(0);
} else {
getSums(args[0]);
if (isValid()) {
System.out.println("Valid ISBN");
} else {
System.out.println("Invalid ISBN");

}
}
}
}


:)

[jemfinch]

Just as a note, inkedmn, you might be better off to put your ints sum1 and sum2 inside the getSums method so your class is threadsafe. As it is, calls in different threads to getSums will result in a big bungling of the sum1 and sum2 variables and some strange results (although you're best off getting someone's opinion who knows Java better)

Jeremy

[inkedmn]

well, sum2 is accessed by more than one method, so i just figured it'd be easier to make them instance variables...


[edit]
just posted an EXTREMELY commented version on ccae.

you know, it's all about the kids...

[jemfinch]

Ok, there's a possibility I'm misunderstanding your code here because of my lack of knowledge of Java, and it actually has an object you have to instantiate every time you want to validate an ISBN. If so, ignore what follows (in the context of your current code).

Originally posted by inkedmn
well, sum2 is accessed by more than one method, so i just figured it'd be easier to make them instance variables...


This should be a signal to you that your functions aren't broken down well; that they're too coupled. Instead of using sum2 in both functions, you should either make the first function return something that the second function takes as an input or combine the two functions into one (with its own internal state).

Jeremy

[inkedmn]

the way it's written, an object is instantiated each time it validates an ISBN...

[jemfinch]

Then I need to learn a little more about Java :)

What would it look like if you were to use the class from a separate class?

Jeremy

[kmj]

The way it's written, it never instantiates an object; and unless there's something about java, only one class can have "main" in it. There may be something that I'm missing though; I'm no guru on the matter.

[stuka]

Jeremy: that Java code is a perfect example of not-really-OO Java code. Notice how the methods are all static - they are class methods, not object methods. Actually, that code sorta implements your earlier suggestion to an extent, though to do it right, there would only be one function call, like ISBNValidator.Validate(number). As written, calling code will have to essentially rewrite the main function from the class.

[inkedmn]

Originally posted by jemfinch
Then I need to learn a little more about Java :)

What would it look like if you were to use the class from a separate class?

Jeremy

like this:

public class ISBN {

public static void main(String[] args) {
if (args.length != 1) {
System.out.println("Usage: ISBN <ISBN>");
} else {
ValidateISBN isbn = new ValidateISBN();
isbn.getSums(args[0]);
if (isbn.isValid()) {
System.out.println("Valid ISBN");
} else {
System.out.println("Invalid ISBN");
}
}
}
}


i suppose i could combine the two methods into one, but whatever.

:)

[stuka]

inedmn: your code wouldn't be called quite like that - all your methods are static (since you set 'em up to be run from main). What you should have would be:public class ISBN {

public static void main(String[] args) {
if (args.length != 1) {
System.out.println("Usage: ISBN <ISBN>");
} else {
ValidateISBN.getSums(args[0]);
if (ValidateISBN.isValid()) {
System.out.println("Valid ISBN");
} else {
System.out.println("Invalid ISBN");
}
}
}
}
Of course, as was mentioned, this is very much NOT a thread-safe method, as it relies on static (class) variables. You could make it much safer with a few changes....

[inkedmn]

true, but my way works too :)

may not be the safest/most efficient way to go about it, but it does what it's supposed to

[stuka]

Won't argue that.

[sans-hubris]

Originally posted by Stuka
Jeremy: that Java code is a perfect example of not-really-OO Java code. Notice how the methods are all static - they are class methods, not object methods. Actually, that code sorta implements your earlier suggestion to an extent, though to do it right, there would only be one function call, like ISBNValidator.Validate(number). As written, calling code will have to essentially rewrite the main function from the class. Well, it's still OO, just not in the logical/organizational sense, which is a possibility with any OO language.

[stuka]

sans-hubris: that's why I said not-really-OO - the exact same thing, with practically the same calling sequence, could be written in any non-OO language. Also, as jemfinch mentioned earlier, this particular problem doesn't really merit a full-on OO solution - it's too small. The best overall design for this would be to create a utility package and include these methods in it (akin to java.Math), IMO.

[jemfinch]

Ah, that's what I suspected when I saw the "static" keyword on all those methods. In that case, my suggestion does apply, and sum1 and sum2 need to be in the function(s) that use them, either as an argument or as a stack variables in the method itself (hmm...that's C nomenclature, but I forget what to call them otherwise :))

Jeremy

[stuka]

jemfinch: I think the word you're looking for is 'automatic' variables...ahh, auto, the forgotten keyword in C.

[bwkaz]

On an entirely unrelated note, there's a professor here who is dead set on believing that auto variables (and the increment and decrement operators as well) are not part of the ANSI C standard. And the fact that gcc and Sun's cc accept them even when they're in -ansi mode (actually, Sun's cc always is, as far as I can tell) is because of bugs in gcc/cc, not the fact that they are actually valid ANSI C constructs. Of course, his favorite compiler, the SGI cc from circa 1979, doesn't compile them, so they must not be ANSI C (ignore the logical fallacies here, please, those aren't the point).

Now I'm not positive, he could possibly be right, but I really doubt it. I'm sure the ANSI C specification is posted somewhere on the Web, but where is it? I'd like to find these things out for myself in case I ever have to deal with that (I don't have him now, but I've heard stories).

[stuka]

The unfortunate fact is that the ANSI/ISO standards aren't available for free - though from what I understand most of the cost is for printing the darn things. Now HOW an auto variable (which is fundamental to C) could NOT be part of the ANSI/ISO standard is beyond me...does this prof believe that all variables must be either global or static?

[kmj]

http://www.cs.colorado.edu/~eliuser/c_html/c.html#m262


google gave me that; can't verify it's validity..

Is there ever any reason to explicitly use the 'auto' keyword? seeing as how all variables not given a specific class are auto by default...

[jemfinch]

Originally posted by Stuka
jemfinch: I think the word you're looking for is 'automatic' variables...ahh, auto, the forgotten keyword in C.

Well, all variables default to automatic, so yes, that is a keyword used in C to say what I'm saying :) I just don't know what the generic terminology is.

Jeremy

[stuka]

oooh...generic....hmmm....I always thought they were called 'local' variables, as there lifetime and visibility were limited to the procedure/function.

[bwkaz]

Originally posted by kmj
http://www.cs.colorado.edu/~eliuser/c_html/c.html#m262

google gave me that; can't verify it's validity..

Google. *smack* I should have tried that. Oh well.

Well, typedef, extern, static, auto, and register, sounds OK to me. It also has ++ and -- there. Hmm. Well, thanks.

[jemfinch]

You know, Stuka, as crazy as it sounds, I think you're right. They're local variables.

Grrr...how I could forget such a thing, I have no idea...

Jeremy

[kmj]

"You know, Stuka, as crazy as it sounds, I think you're right. They're local variables."

Aw.. come on! Stuka's been right before. Once or twice. Hasn't he? Haven't you stuka? I mean, you have been right before at least once, right? I mean, in your whole life... no? not at all? oh... sorry.

(j/k)

[jemfinch]

...As crazy as it sounds [that I forgot the term "local variable"]...

(Just to make my intent clear so I doubt sound like a jerk :))

Jeremy

[kmj]

:D