# Question on Pset2 Caeser

I am a beginner to coding and a rising ninth grader. This is my code on the bottom and I ran check50 on it and everything was green but, I'm not sure if the code would be consider as "right" because I think I might've done it in an unusual way. The part that I am unsure about is how I encipher the letter. Like this: c[i] = ((p[i] + k) % 26) + 97; Is my code okay? Thanks you

``````
string p;
string c;
int character;
int k;
int main(int argc, string argv[])
{
if (argc < 2 || argc >= 3)
{
printf("Usage: ./caeser key\n");
return 1;

}

if(argv[1] != NULL)
{
for (int i = 0; i < strlen(argv[1]); i++)
{
if(isdigit(argv[1][i]) != 0)
{
if(i == strlen(argv[1]))
{
break;
}

else
{
continue;

}

}

else if(isdigit(argv[1][i]) == 0)
{
printf("Only integers\n");
return 1;
}

}

}

if(argc == 2)
{
k = atoi(argv[1]);

}

c = p = get_string("Plaintext: ");

for (int i = 0; i < strlen(p); i++)
{
if(isalpha(p[i]) != 0)
{
if(isupper(p[i]) > 0)
{
p[i] = p[i] - 65;
c[i] = ((p[i] + k) % 26) + 65;

}

if(islower(p[i]) > 0)
{
p[i] = p[i] - 97;
c[i] = ((p[i] + k) % 26) + 97;

}
}

}

printf("ciphertext: %s\n", c);

return 0;

}

``````

One of the things you'll learn in programming is that there are usually many "right" ways to do things. Later, you'll also see a lesson on programming efficiency. The question shifts from what's right to what's more efficient. And then, it gets even more interesting because you have to define what's more important, efficient programming, efficient runtime execution, efficient memory usage, efficient resource usage, etc. It's often a tradeoff or a balance of which is more important. For example, one program might need to run as fast as possible, but it doesn't matter how much memory is used, while another program might require the most efficient use of limited memory while speed doesn't matter.

OK, having said all that, let's look at your code and make it "better". It's still right, but let's go for more right. ;-)

``````if(isalpha(p[i]) != 0)
{
if(isupper(p[i]) > 0)
``````

First, the calls to isalpha and isupper. In each case, the code compares to 0. All of the issomething() function calls will return a 0 when false and a power of 2 when true and 0 when false. (Want to learn something? Write a program that calls all of the is*() functions and prints the return values when true. Each returns a different number.) If you read the spec, it says that they all return an int, not a bool. Now, note that C always interprets 0 as false and a non-zero (positive or negative) as true. How can you use that? Simple. Instead of doing a comparison to 0, just use a straight test:

``````    if(isupper(my_char)
``````

This elininates the additional comparison to 0 and makes the if statement more efficient by eliminating an operation and a constant from the line of code!

Next, there's this:

``````        p[i] = p[i] - 65;
c[i] = ((p[i] + k) % 26) + 65;
``````

This code is correct. It's also very readable. Want to make it execute more quickly? Combine the two lines.

``````        c[i] = ((p[i] - 65 + k) % 26) + 65;
``````

It will be slightly more efficient at runtime, but less readable.

Which is more important? If it only needs to run a few times or occasionally, leave it as-is. But if it were in a production system running millions of times an hour, any improvement in efficiency is important. As the programmer, you'd need to decide which is more important.

More tips.

Else clauses are optional. When it's going to do nothing, it's not needed. If all it does is `continue`, delete the entire clause.

Input parameter validation.

``````if (argc < 2 || argc >= 3)
``````

This makes two separate tests. Only one test is really needed:

``````if (argc != 2)
``````

This checks that argc is really 2, and if not, it prints an error message and terminates the program. And since argc either is or isn't 2, there's no need to check if it is 2 later.

There's another piece of logic that follows from this. If argc is 2, then argv[1] simply must be non-null. So that's another test that isn't needed.

Now, in the following block:

``````if(argv[1] != NULL)
{
for (int i = 0; i < strlen(argv[1]); i++)
{
if(isdigit(argv[1][i]) != 0)
{
if(i == strlen(argv[1]))
{
break;
}

else
{
continue;
}
}

else if(isdigit(argv[1][i]) == 0)
{
printf("Only integers\n");
return 1;
}
}
}
``````

The first half of this code block is what is known as dead code. It does nothing and can be deleted without affecting the programming. A little more altering of the code and it will be much more efficient.

``````for (int i = 0; i < strlen(argv[1]); i++)
{
if( !isdigit(argv[1][i])  )
{
printf("Only integers\n");
return 1;
}
}
``````

This is your code with all the inefficiencies removed and the slightest alteration of the if condition. Doesn't this look a lot cleaner?

While your code is "right" there was a lot of room for improving efficiency. It seems that you've gotten the impression that all aspects of if/then/else is necessary as well as comparisons in logical tests. Often, they're not needed at all and can and should be removed!

I've gone into all this detail to get you on the right track, and because you're a young programmer with a lot of potential. You're doing well, and you'll get the hang of all of this.

Just remember, especially now while you're learning, to do this. After you write a program, go back and ask yourself, "Is there a better way to do this?" Often, you'll find that there is, especially if you have the feeling that something is overly complex or too long. ;-) Also, if you wait a day or two after you've written the code, and have had a chance to "sleep on it", you'll come back and see ways to improve code a lot easier. Eventually, the best practices and more efficient techniques will become second nature!

So, as you can see, there's more than one "right" way to do just about anything. It's all about the priorities in the program design.

One final programming tip. In this code, you've declared several global vars - p, c, character, and k. This is a very bad habit. Variables should never be created as globals unless there's a really good reason to do so. (Some people say "never, ever". Good programmers know that they have their place, but should only be rarely used. ) When there are no functions in a program and only main, there definitely should never be a global var.

The reason is this: variables should be contained to either main or the function in which they are used and nowhere else. When there is a global, there's a good chance that the variable name could be inadvertently used inside a function where it isn't intended to be used. This can lead to software bugs, or the creation of something called a shadow variable (a local var that masks or hides a global var.) So, unless a variable really needs to exist across all functions and main, for the life of a program's execution, they should not be created. Local vars should be used instead. Always think of a global var as a big red flag! (You'll see a case where globals are appropriate later.)

Happy programming!!! ;-D

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)

• Thank you for the tips and the very thorough answer! – Reeman Jul 28 '20 at 13:06

The video on the assignment gave hints towards using that approach. I can't tell you it's "right" or "the best", but it makes perfect sense.