10
\$\begingroup\$

This is the first program I have ever written outside of what I am learning in class. I am in my first programming class (ever) in college and I'd like to know if this is any good or if I'm making glaringly bad habits and mistakes. I am aware that some of the code I've written is redundant, but I don't think I've quite learned enough to fix it.

This program was created using Xamarin on my MacBook Pro, so the file structure built into the code is different from Windows and will have to be changed if you try to test it:

class if // under method FOR, change the string mainpath path to something windows

using System;
using System.IO;

namespace FileCreator2
{
    public class MainClass
    {
        public static void Main (string[] args)
        {
            int counter = 1;
            string useridtext = "NO VALUE";
            string passtext = "NO VALUE";
            string mainpath = "NO VALUE";
            string answer = "NO VALUE";
            string startup = "You have already created a username and password.";

            If ifs = new If ();

            ifs.FOR (counter, mainpath, startup, useridtext, passtext, answer);
        }
    }
    public class determiner
    {
        public void FilesExist(string mainpath, string startup, string useridtext, string passtext)
        {
            pause Pause = new pause();
            directory Direct = new directory ();

            if (Directory.Exists (mainpath))                                                                                
            {
                Console.WriteLine (startup);
            } 
            else 
            {
                Console.WriteLine ("Create your username: ");
                useridtext = Console.ReadLine ();

                Direct.Create (useridtext, passtext, mainpath);

                Console.Clear ();

                Console.WriteLine ("Create your password: ");
                passtext = Console.ReadLine ();

                Direct.Create (useridtext, passtext, mainpath);

                Pause.Input ();
                Console.Clear ();
            }
        }
    }
    public class If
    {
        public void statement(string answer, string mainpath)
        {
            delete Del = new delete ();
            pause Pause = new pause ();

            if (answer == "Y") 
            {
                Del.Files (mainpath);
                Console.WriteLine ("Files deleted.");
            } 
            else if (answer == "y") 
            {
                Del.Files (mainpath);
                Console.WriteLine ("Files deleted.");
            } 
            else if (answer == "N")
            {
                Console.WriteLine ("Here is the text in the file you created:");
                Console.Write ("Username: ");
                Console.WriteLine (File.ReadAllText (mainpath + "/userid.txt"));
                Console.Write ("Password: ");
                Console.WriteLine (File.ReadAllText (mainpath + "/passwd.txt"));
                Pause.Input ();
            }
            else if (answer ==  "n")
            {
                Console.WriteLine ("Here is the text in the file you created:");
                Console.Write ("Username: ");
                Console.WriteLine (File.ReadAllText (mainpath + "/userid.txt"));
                Console.Write ("Password: ");
                Console.WriteLine (File.ReadAllText (mainpath + "/passwd.txt"));
                Pause.Input ();
            }
            else 
            {
                Console.WriteLine ("Invalid answer. Files not deleted, program terminated.");
            }
        }
        public void FOR(int counter, string mainpath, string startup, string useridtext, string passtext, string answer)
        {
            determiner determine = new determiner ();
            If ifs = new If ();

            for ( ; counter > 0; counter++) 
            {
                mainpath = @"/Users/kevgreenan/Desktop/PROJECT"; //this MUST change for WINDOWS users

                determine.FilesExist (mainpath, startup, useridtext, passtext);

                Console.WriteLine ("Would you like to erase the data (Y/N)?");
                answer = Console.ReadLine ();
                ifs.statement (answer, mainpath);
                Console.Clear ();

                Console.WriteLine ("Would you like to exit (Y/N)?");
                answer = Console.ReadLine ();
                if (answer == "Y") 
                {
                    counter = counter - 999999;
                }
                else if (answer == "y") 
                {
                    counter = counter - 999999;
                }
                else if (answer == "N")
                {
                    // this will start the statement over
                }
                else if (answer == "n") 
                {
                    // this will start the statement over
                } 
                else 
                {
                    Console.WriteLine ("Invalid input.");
                }
            }
        }
    }
    public class directory
    {
        public void Create(string useridtext, string passtext, string mainpath)
        {
            System.IO.Directory.CreateDirectory (mainpath);
            System.IO.File.WriteAllText (mainpath + "/userid.txt", useridtext);
            System.IO.File.WriteAllText (mainpath + "/passwd.txt", passtext);
        }
    }

    public class delete
    {
        public void Files(string mainpath)
        {
            Directory.Delete (mainpath, true);
        }
    }
    public class pause
    {
        public void Input()
        {
            Console.WriteLine("Press any key to continue. . .");
            Console.ReadKey();
        }
    }
}
\$\endgroup\$
0
5
\$\begingroup\$

Magus has already cleared the naming aspects so I won't talk about them. Except for this: drop everything and review your naming. Both conventions on how you structure them and actually thinking about whether or not the names for classes/variables really convey what they're about. At first glance, I have no idea what your code is about.

Onto something more functional:

The DRY Principle

DRY - Don't Repeat Yourself - is a very important aspect of creating a maintainable application. It makes sure that when you have multiple portions of code that do the same thing and you need to change that thing, that you will only have to make 1 change and not change it at every place in the code.

What if I want to change the default value to "NO VALUE LOL"? In your scenario, I'd have to change it 4 times.

Instead, look towards something like this:

string defaultValue = "NO VALUE";
string useridtext = defaultValue;
string passtext = defaultValue;

And now that issue is solved.

You do the same when checking if the input is "y"/"Y" or "n"/"N". Here the solution is not as straightforward, but you can make this more readable and maintainable by doing something like this:

if(answer == "n" || answer == "N")

or even better:

if (answer.Equals("n", StringComparison.InvariantCultureIgnoreCase))

More about string comparison


Getting loopy

for ( ; counter > 0; counter++) 
if (answer == "Y") 
{
    counter = counter - 999999;
}

This is a very hacky way of making an infinite loop. On the bright side though: you have the right idea!

Let me first point out what's wrong with this approach.

  1. Can you tell just by looking at the code whether or not what the intention was? Probably, but it's not as evident.

  2. What happens if I'm really, really, really clumsy and I gave invalid input the first 1 million attempts but after that I get it right? It still won't end!

A better way of having an infinite loop is like this:

while(true)
{


   if(JeroenIsAwesome)
   {
       break;
   }
}

Obviously this will break from the first iteration. A slight note about this blueprint: a statement of the form if(condition) is evaluation as if(condition == true), it's just shorthand. The break; statement will exit the first enclosing loop it encounters, essentially ending the infinite loop.


Namespaces? Boooooring!

You have using statements available, use them. Using System.IO.File only serves a point when you have to distinguish between another class called File. Since you don't have that, all it does is make the code look more cluttered.


Shorthands

You can change this

counter = counter - 999999;

into this

counter -= 999999;

It's pretty cool. More on that here.


User Experience

Yep, you have to take them into account sometimes. This won't be very helpful for them to have it hardcoded:

mainpath = @"/Users/kevgreenan/Desktop/PROJECT"; //this MUST change for WINDOWS users

Instead, allow them to change it somehow. Either by entering it in the console or by passing it as a command line argument.

This is also begs the question: why pass it around as an argument when you overwrite it in that function anyway?


Proper API

The API - Application Programming Interface - is made up of all methods you create. Typically you are interested in the public API which is what it says: the method calls that you can access as an outsider. As the developer of the API, you want to make sure that what you expose is something the outsider should be using and you avoid giving them access to stuff they should keep their paws off.

Now, in your situation it matters less but imagine someone is going to use your code and work with it. Does he need access to the If#statement() method, or does he need access to MainClass#Main()?

More on that here.

\$\endgroup\$
9
  • \$\begingroup\$ if you want to name a variable default, it should be prefixed with @ since it is a reserved keyword! \$\endgroup\$ – IEatBagels Oct 14 '14 at 17:48
  • \$\begingroup\$ @TopinFrassi: aha, right! I didn't think too much of it and figured the syntax color was borked somewhere. \$\endgroup\$ – Jeroen Vannevel Oct 14 '14 at 17:48
  • \$\begingroup\$ It's not much of a worry, but I think that the OP might not know about the @ prefix \$\endgroup\$ – IEatBagels Oct 14 '14 at 17:49
  • \$\begingroup\$ @TopinFrassi: That's likely true, but it is very rare for it to be a good idea to use it. There are cases when you'll want to use the name of a reserved word, but you can usually think of a name which gets the point across without the potential confusion. It's not something I'd encourage explaining to someone in their first programming course, especially since it would make some of those class names easier. \$\endgroup\$ – Magus Oct 14 '14 at 18:25
  • \$\begingroup\$ For the logical-OR operand, I tried doing something like if (answer == "string1" || "string2") got an error and then moved on to what I knew would work. Xamarin adds namespaces by default, I didn't clear it because I got lazy. And as far as the mainpath variable, I was considering using some kind of OS detector (I think there is a method built in) with an if statement. My biggest question is am I using too many if statements? I felt like I over used them and could have somehow lumped them into one method. I was trying to follow the "rule" of having short(ish) methods. \$\endgroup\$ – Kevin Greenan Oct 14 '14 at 18:26
7
\$\begingroup\$

First off, well done! Many first questions on this site don't even contain code which compiles!

Now for the review:

  1. In C#, class names are expected to be in PascalCase. This helps your code look more like the surrounding framework code, thereby being more familiar to other programmers.
  2. Likewise, local variables are expected to be camelCase
  3. Classes are meant to hold some data and some behavior in most cases. They are typically named with nouns, because they represent objects. As such, delete is not a good name for a class; it's a verb, and a very good choice for a method name, provided it's obvious what is being deleted.
  4. On the same token, nouns make lousy method names. Imagine the sentence "I will Files a delete." - that is how your code will be read.

A survey was once made to determine the hardest part of programming. Naming won. Generally, when you extract a method, it should have a name that conveys a bit of what it's doing. Calling a method on something named If does not tell you much about what that code is doing. You'd expect it to return true or false based on some kind of input, whereas yours is simply a wrapper for arbitrary conditional code. Consider what you'd call that step of processing, and use that name.

Overall, good work. Many people take entry level classes and cannot write a working for loop. Just the fact that you are seeking feedback this early on shows a lot of promise.

\$\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.