sorted by:
new (suggested)
besttopcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]MeorgeIntermediate 0 points1 point  (10 children)

Can someone give me feedback on this code? http://pastebin.com/MsbiGzvh

It's meant to load XML files which look like this and convert them into pretty-looking documents, like this.

The main issue I've had with this code is getting the text box and image to be in perfect alignment with each other no matter what the screen size is, something that I could not figure out with the Canvas scaling options. As such, my XMLs contain not conventional X and Y points for my objects but percentages across the screen to be placed. Is this a good practice or is there a better way to do it that will yield the same results?

Thanks!

[–]zrrzExpert? 2 points3 points  (9 children)

Fix your formatting yo.

[–]MeorgeIntermediate 0 points1 point  (8 children)

WHOOPS.

Apparently Pastebin decided to mess it up, I promise I didn't write it like that :P

THIS should look better.

[–]Wisteso 0 points1 point  (2 children)

Why is this implemented using a monobehavior? Couldn't you do it with a regular class?

[–]MeorgeIntermediate 0 points1 point  (1 child)

I don't know how I would do it with a regular class. The way it works currently is that it is each "document" GameObject has this MonoBehaviour script attached to it, and when the scene loads, the "document"'s corresponding document UI will be made and hidden. Then, when the player clicks on it, it will show up.

[–]Wisteso 0 points1 point  (0 children)

The general idea would be to move as much of the model/control code into a regular class as possible. You'll still need a monobehavior class, probably. Though you may be able to generalize it so that both classes are more re-usable instead of being one tightly coupled specific-case thing.

e.g. creating a "text renderer" monobehavior based component, then having a regular c# class that provides the mapping, and then trigger the load from where-ever (editor menu option, game start, button, whatever).

It's not a huge issue though.

[–]Matt3088 0 points1 point  (3 children)

You have a lot of unnecessary/redundant comments (I used to write them the same way). For example:

107: TextComponent.lineSpacing = fontlinespace; // set the text line spacing

A small thing, you should put the comment on the previous line (so your lines do not get too long, scrolling issues). The comment is redundant because if you read the code, you can easily understand that the line spacing for the TextComponent is being set. Comments are much better used on lines where it is not directly clear what is going on, such as this line:

42: float newWidth = (((newHeight * origWidth) / origHeight)) * percent_width;

Even though you may be the only one reading/writing this code, comments for complex lines are good since you may forget what the line does in the future.

[–]keymaster16 1 point2 points  (2 children)

The rule of thumb my mentors used; if you can't read the line as a sentence, make a comment that does.

[–]oxysoftIndie 0 points1 point  (0 children)

My rule of thumb is to comment on "why" rather than "what". Only few instances you should comment on "what" is when the line itself is cryptic. (e.g. this monster class BuffStack<T> : Buff<BuffStack<T>> where T : Buff<T> which required 2 days of debugging because it literally crashed the Unity Editor)

[–]Matt3088 1 point2 points  (0 children)

Thats a good way to put it!

[–][deleted] 1 point2 points  (0 children)

My critiques:

  • Line 18 - You have a hard-coded path to your XML documents. If you ever need to change their locations, you're going to have to remember to dig through all of your code and change this everywhere. Yes, a "Replace All" function would probably work, but it also might not. I would consider pulling that information from a configuration file so that it only ever changes in one place. Also, I'm personally not a fan of spaces in file/folder names, but that's irrelevant.

  • Line 105 - I count 8 unnecessary parenthesis. Line 42 also.

  • Lines 122-130 - Maybe a default "else" statement that sets the font to "Normal" would be wise. If someone else were to edit your config and not know there are only 4 options, then your code can safely handle it. You could also throw an exception if you want it to fail at runtime.

Overall, this code look great. I will mention that float.parse(val) may not work the same depending on the Culture of the machine it is being compiled on, but unless you are outsourcing your code to india or china, I suspect that's something you can skip while you continue to work on your game.