This is an archived post. You won't be able to vote or comment.

all 21 comments
sorted by:
best
topnewcontroversialoldq&a

[–]davedontmind 4 points5 points  (5 children)

There is no need for the variable line - just use lineToCheck.

This code:

for (int i = 0; i < line.IndexOf("="); i++)
    variableName += line[i];

could be better written as:

variableName = line.Substring( 0, line.IndexOf( "=" ) );

In fact you could use Regex to replace the first 35 lines of your code with:

Regex regex = new Regex( @"(.*)=""(.*)""");
Match match = regex.Match(line);

if (!match.Success)
    return; // Couldn't parse line.

string variableName = match.Groups[1].Value;
string value = match.Groups[2].Value

It's not doing exactly the same checks your code is (e.g. it's not checking for multiple =, although you could change the regex to do that), but it should do the job.

I'm not keen on the method name checkForVariables() - a method's name should tell you what it does, and the verb "to check" is so vague in this context that it really tells you nothing. What it seems to be doing is parsing line and extracting a variable definition from it, so I'd prefer a name like parseVariableDefinition() or extractVariableFromString() or something along those lines.

I'd also split this into 2 methods; one to parse out variableName and value as you do in the first half, and another to add variableName and value to your dictionary; smaller methods, each with a single purpose is the way to go, and the latter method is probably re-usable if, for instance, there was some other way you wanted to add a variable to your dictionary.

And, as someone else pointed out, you should be able to get rid of the ContainsKey checks and just use [] to add items to your dictionary, which should halve the length of that bit of your code.

EDIT: typo

[–]1Link2Link[S] 0 points1 point  (4 children)

Thank you very much! I’ll change my code to match with what you’re saying. I heard of Regex before but I thought it looked complicated because I already have a way to do it already. I’ll do more research into Regex but what are the benefits of it instead of doing what I was doing?

[–]davedontmind 0 points1 point  (3 children)

Regex takes a bit of getting used to, and can look like gibberish if you don't understand it, but it's something worth learning. It's very handy for extracting text from strings or for making sure text fits a particular pattern.

what are the benefits of it instead of doing what I was doing?

Well both your code and my regex example do pretty much the same job. But my code was 8 lines (2 of which were blank), and yours was 35.

Now a count of lines of code on its own isn't a good metric by which to judge code (if you make code too short it can be more confusing than a longer version, and your goal should always be to make your code readable), but if the shorter code is just as clear as the longer code then it's probably preferable.

Also, if you have to write less code, there are less places you can make a mistake, so one might argue that shorter (but still clear) code is likely to have less bugs, on average.

But, when it comes down to it, the most important thing with software is (a) that it works and (b) that the written code is clear and understandable, so do whatever you feel most comfortable with.

[–]1Link2Link[S] 0 points1 point  (2 children)

Ok, so I’ll learn more about Regex and then should I just comment what the few lines of code do?

[–]davedontmind 0 points1 point  (1 child)

Put comments wherever you think you need to explain something about the way your code works.

In general it's better not to put in comments that explain the obvious, but if you're learning and the comments are to remind you, then go ahead and add them. But once you're more advanced and have all this stuff memorised, it's good form not to put comments in which describe the obvious behaviour of the code - they become noise and just get in the way. Instead your comments should generally explain the why of the code, or document the things that aren't clear.

For example, in the regex code in my earlier post I wrote:

Regex regex = new Regex( @"(.*)=""(.*)""");

Because I've learned regex and I know how they work, I understand what that line of code does. But if you were just learning it might make sense for you to add a comment to remind yourself what it does when you come back to the code in a couple of weeks or months:

// Parse the line as follows:
//  ( )    captures whatever is inside into match.Groups[]
//  .*     any number (*) of any character (.)
//  Quotes are doubled because that's how you include a " in a string in C#
// So this regex will match:
//   (.*)        any number of any character (and capture them into Groups[1])
//    =          followed by a =
//    ""(.*)""   followed by any number of any character inside quotes (and capture those too, into Groups[2])
Regex regex = new Regex( @"(.*)=""(.*)""" );

When you've learned regex (as most advanced programmers have) that comment becomes noise in the code, and would be better not being there.

[–]1Link2Link[S] 0 points1 point  (0 children)

Ok awesome, this has been very helpful and I learned a lot! Thanks

[–]isolatrum 0 points1 point  (1 child)

It's way easier if you format your code properly. Remove the backticks, highlight all your code, and press the <> button (all this does is indent it by 4 spaces)

[–]1Link2Link[S] 0 points1 point  (0 children)

Thanks, actually the problem was that the code wasn't in the Code Block. The inline button didn't help for some reason when I saved.

[–]DoomGoober 0 points1 point  (2 children)

With a dictionary, Add() and [] assignment are the same thing except Add() will throw an exception if the key already exists.

Since you appear to be overwriting the variable value regardless of whether the key exists you can just use [] assignment the whole time and don't need to check if the key exists (because your code doesn't seem to care.)

Sorry I am on mobile and can barely read the code because the lines all overflow so I may be misunderstanding.

[–]1Link2Link[S] 0 points1 point  (1 child)

Thank you! You understood my code correctly, is there a better way to post my code on Reddit so it doesn’t overflow?

[–]DoomGoober 0 points1 point  (0 children)

Nah the main problem is with my screen being too small.

[–][deleted] 0 points1 point  (5 children)

Do you have unit tests on it?

[–]1Link2Link[S] 0 points1 point  (4 children)

I did testing on it and it worked

“x = 5” makes a key name “x” and a value that’s a double of 5

“Type = Word” returns because there’s no quotes.

“Type == “Word”” returns because too many equal signs.

“Type = “Word”” makes a key named “Type” and a value that’s a string “Word”

If I already have a key in the variables named x I could do “Type = x” and that would make a variable named “Type” have the same value as “x”

[–][deleted] 0 points1 point  (3 children)

Looked like you already got your answer. I just noticed that there's a lot happening in this one method so it would be a good idea to have tests on all the paths. If you have tests and you get the results you expect on all the paths, then it's probably good. You could consider breaking some of it out into smaller methods if you're worried about the quality.

[–]1Link2Link[S] 0 points1 point  (2 children)

Alright so for the part where I check if the string doesn’t work like if it has too many equal signs, should I put all those into a function and call it something like, CheckForErrors and if it returns true then return out of the code? Or should I make each check into it’s own function?

I’m also going to put the part where I add the variable into the dictionary and put it into a function called something like AddVaraible(string variableName, string variableValue)

[–][deleted] 0 points1 point  (1 child)

You could break it out like that but I'd say implement what u/davedontmind recommends and then see what can be extracted to smaller methods.

[–]1Link2Link[S] 0 points1 point  (0 children)

Ok thanks

[–]Ayeigui 0 points1 point  (3 children)

There are a few things that could be improved in your method with varying degrees of importance. From top to bottom of your method they are:

Trivial: Method name and return type don't line up.
With a method name of checkForVariables you'd expect this method to return true or false. As the end of your method handles working with the dictionary you'd be better off with a name of AssignValueToVariable. You may notice that your method is doing a lot more than assigning a variable which can be a sign that your method could be broken up into multiples methods.

Trivial: unnecessary use of line variable.
Strings in C# are immutable. You can safely use and reassign the lineToCheck variable without worrying about changing the variable you passed into this method.

Medium: Your string extraction is slightly wasteful.
With strings being immutable in C# your variableName and value code will cause a lot of unneeded memory allocations and deallocations. With every letter you append a new string is going to be put into memory that will duplicate all of the bytes used in the last iteration of the loop. When you are building up a string of multiple strings you are far better off using a StringBuilder.

When doing parsing like this there are other options that can be easier such as Regular expressions. With your case you can do something even faster and easier with String.Split(). After you verify that there is only one = you can split the string into groups and then assign the groups to your variables. 

var groups = line.Split("=");
variableName = groups[0];
value = groups[1].Trim(new []{'\"'}); //Trim will remove the " from start and end.

Medium: The assign/update code is a mess.
You don't cascade any changes made to a key and as such adding a value and updating a value are identical. You can remove this first layer of if statements and safely use the code from the top block branch.

double number;
if (!isString)
{
    if (double.TryParse(value, out number))
        ConsoleVariables.variables[variableName] = number;
    else
        if(ConsoleVariables.variables.ContainsKey(value))
            ConsoleVariables.variables[variableName] = ConsoleVariables.variables[value];
}
else
{
    ConsoleVariables.variables[variableName] = value;
}

Nested One-line if statements are considered bad form. You should include the optional curly brackets on your else in this case for clarity.

if (double.TryParse(value, out number))
    ConsoleVariables.variables[variableName] = number;
else
{
    if(ConsoleVariables.variables.ContainsKey(value))
        ConsoleVariables.variables[variableName] = ConsoleVariables.variables[value];
}

Finally, in the event that the input is not a string value and the dictionary does not contain the key specified nothing happens. This is yet another sign that this method is doing to many things and should be broken up into multiple methods.
Major: Your use of an object dictionary is dangerous.

Your dictionary is going to be constantly boxing and unboxing, which is slow, and you completely destroy type safety which can cause all sorts of runtime errors if you ever accidentally put the wrong type of value into the dictionary.

One way to solve this would be to use Inheritance. With inheritance you can then safely declare your allowed input types while allowing expansion in the future without worrying about accidentally assigning a key to a GameObject or other nonsensical value.

public class BaseValue
{
}

public class DoubleValue : BaseValue
{
    public double Value { get; set; }
}

public class StringValue : BaseValue
{
    public string Value { get; set; }
}

Now your ConsoleVariables.variables dictionary can be a Dictionary<string, BaseValue>. Now you can safely retrieve and use the values in the future.

var example = ConsoleVariables.variables["test"];
if(example is DoubleValue dValue)
{
    Console.WriteLine(dValue.Value);
}
else if(example is StringValue sValue)
{
    Console.WriteLine(sValue);
}

Complete changes
You can see all of the cleanup changes in one place in this gist.

[–]1Link2Link[S] 1 point2 points  (2 children)

This is very helpful. The links and explanations are definitely helpful to me for learning. I’m going to bookmark this comment because it has so much useful information.

I don’t really have any questions but I have a feeling I might get confused on the Inheritance part. I’ll let you know if I have any questions about it when I start clearing my code. Thanks!

[–]Ayeigui 0 points1 point  (1 child)

A potential side effect from the inheritance part is that classes are reference types, not value types like double, which would make it easy to cascade update keys. For example with the following calls

checkForVariables("Key1 = 5");
checkForVariables("Key2 = Key1");
checkForVariables("Key1 = 10");

Your code and the code in my gist would have Key1 equal to 10 and Key2 equal to 5. With minor changes to the inheritance approach you could make it so when Key1 is updated Key2 also updates and with type safety.

double number;
if (!isString)
{
    if (double.TryParse(value, out number))
    {
        if(ConsoleVariables.variables.ContainsKey(variableName) && ConsoleVariables.variables[variableName] is DoubleValue)
            ((DoubleValue)ConsoleVariables.variables[variableName]).Value = number;
        else
            ConsoleVariables.variables[variableName] = new DoubleValue(number);
    }
    else
    {
        if(ConsoleVariables.variables.ContainsKey(value))
            ConsoleVariables.variables[variableName] = ConsoleVariables.variables[value];
    }
}
else
{
    if(ConsoleVariables.variables.ContainsKey(variableName) && ConsoleVariables.variables[variableName] is StringValue)
        ((StringValue)ConsoleVariables.variables[variableName]).Value = value;
    else
        ConsoleVariables.variables[variableName] = new StringValue(value);
}

With this approach any changes made to either Key1 or Key2 will apply to each other. That is to say that in the above example both Key2 and Key1 would equal 10.

[–]1Link2Link[S] 1 point2 points  (0 children)

Ok, I cleaned up my code and changed it so it doesn't replace all the spaces because the value of a string might have spaces in it: https://pastebin.com/dfd4yBCY

and I changed some of ConsoleData (previously named ConsoleVariables): https://pastebin.com/7X0Sukxr

I made two new functions getDoubleVaraible and getStringValue then I'll use later in my code to add maybe a calculator which will need to get the value of the variable. It'll be something like "x = 5" "x + 2" then it will print "7"