Since there are a fair number of solutions I'm going to go with the "critique" part of your question. A couple of notes: I've fixed some typos and noted where I did. If I'm wrong about them being a typo mention it in the comments and I'll explain what's going on. I'm going to point out several things that you may already know, so please don't take offense if I did. Some comments may seem picky but I don't know where you are in your journey so have to assume you are just starting out.
CREATE function Palindrome (
@String Char
, @StringLength Int
, @n Int
, @Palindrome BIN
, @StringLeftLength Int
ALWAYS include the length with a char
or varchar
definition. Aaron Bertrand talks about it in depth here. He is talking about varchar
but the same goes for char
. I'd use a varchar(255)
for this if you only want relatively short strings or maybe a varchar(8000)
for larger ones or even varchar(max)
. Varchar
is for variable length strings char
is only for fixed ones. Since you aren't sure of the length of string being passed in use varchar
. Also it's binary
not bin
.
Next you don't need to put all of those variables as parameters. Declare them within your code. Only put something in the parameter list if you plan on passing it in or out. (You'll see how this looks at the end.) Also you have @StringLeftLength
but never use it. So I'm not going to declare it.
The next thing I'm going to do is re-format a bit to make a few things obvious.
BEGIN
SET @n=1
SET @StringLength = Len(@String) -- Missed an @
WHILE @StringLength - @n >1
IF Left(@String,@n)=Right(@String, @StringLength) -- More missing @s
SET @n = @n + 1 -- Another missing @
SET @StringLength = @StringLength - 1 -- Watch those @s :)
RETURN @Palindrome = 1 -- Assuming another typo here
ELSE
RETURN @Palindrome =0
END
If you look at the way I did the indenting you'll notice that I have this:
WHILE @StringLength - @n >1
IF Left(@String,@n)=Right(@String, @StringLength)
SET @n = @n + 1
That's because commands like WHILE
and IF
only affect the first line of code after them. You have to use a BEGIN .. END
block if you want multiple commands. So fixing that we get:
WHILE @StringLength - @n > 1
IF Left(@String,@n)=Right(@String, @StringLength)
BEGIN
SET @n = @n + 1
SET @StringLength = @StringLength - 1
RETURN @Palindrome = 1
END
ELSE
RETURN @Palindrome = 0
You'll notice that I only added a BEGIN .. END
block in the IF
. That's because even though the IF
statement is multiple lines long (and even contains multiple commands) it is still a single statement (covering everything performed in the IF
and the ELSE
portions of the statement).
Next you'll get an error after both of your RETURNs
. You can return a variable OR a literal. You can't set the variable and return it at the same time.
SET @Palindrome = 1
END
ELSE
SET @Palindrome = 0
RETURN @Palindrome
Now we are into logic. First let me point out that the LEFT
and RIGHT
functions you are using are great, but they are going to give you the number of characters you pass in from the requested direction. So let's say you passed in the word "test". On the first pass you are going to get this (removing variables):
LEFT('test',1) = RIGHT('test',4)
t = test
LEFT('test',2) = RIGHT('test',3)
te = est
Obviously that isn't what you expected. You would really want to use substring
instead. Substring lets you pass in not only the starting point but the length. So you would get:
SUBSTRING('test',1,1) = SUBSTRING('test',4,1)
t = t
SUBSTRING('test',2,1) = SUBSTRING('test',3,1)
e = s
Next you are incrementing the variables you use in your loop only in one condition of the IF statement. Pull the variable incrementing out of that structure entirely. That is going to require an additional BEGIN .. END
block, but I do get to remove the other one.
WHILE @StringLength - @n > 1
BEGIN
IF SUBSTRING(@String,@n,1) = SUBSTRING(@String, @StringLength,1)
SET @Palindrome = 1
ELSE
SET @Palindrome = 0
SET @n = @n + 1
SET @StringLength = @StringLength - 1
END
You need to change your WHILE
condition to allow for the last test.
WHILE @StringLength > @n
And last but not least, the way it stands now we don't test the last character if there are an odd number of characters. For example with 'ana' the n
isn't tested. That's fine but it does me we need to account for a single letter word (if you want it to count as a positive that is). So we can do that by setting the value up front.
And now we finally have:
CREATE FUNCTION Palindrome (@String varchar(255))
RETURNS Binary
AS
BEGIN
DECLARE @StringLength Int
, @n Int
, @Palindrome binary
SET @n = 1
SET @StringLength = Len(@String)
SET @Palindrome = 1
WHILE @StringLength > @n
BEGIN
IF SUBSTRING(@String,@n,1) = SUBSTRING(@String, @StringLength,1)
SET @Palindrome = 1
ELSE
SET @Palindrome = 0
SET @n = @n + 1
SET @StringLength = @StringLength - 1
END
RETURN @Palindrome
END
One last comment. I'm a big fan of formatting in general. It can really help you to see how your code works and help to point out possible mistakes.
Edit
As Sphinxxx mentioned we still have a flaw in our logic. Once we hit the ELSE
and set @Palindrome
to 0 there is no point in continuing. In fact at that point we could just RETURN
.
IF SUBSTRING(@String,@n,1) = SUBSTRING(@String, @StringLength,1)
SET @Palindrome = 1
ELSE
RETURN 0
Given that we are now only using @Palindrome
for "it's still possible this is a palindrome" there is really no point in having it. We can get rid of the variable and switch our logic to short circuit on failure (the RETURN 0
) and RETURN 1
(a positive response) only if it makes it all the way through the loop. You'll notice this actually simplifies our logic somewhat.
CREATE FUNCTION Palindrome (@String varchar(255))
RETURNS Binary
AS
BEGIN
DECLARE @StringLength Int
, @n Int
SET @n = 1
SET @StringLength = Len(@String)
WHILE @StringLength > @n
BEGIN
IF SUBSTRING(@String,@n,1) <> SUBSTRING(@String, @StringLength,1)
RETURN 0
SET @n = @n + 1
SET @StringLength = @StringLength - 1
END
RETURN 1
END
LTRIM(RTRIM(...))
whitespace?