Click here to Skip to main content
15,886,689 members
Please Sign up or sign in to vote.
1.64/5 (7 votes)
See more:
asasssasdasndsndjnsajdfnasjndasndjasnjdsjdnsnds
Posted
Updated 5-Dec-14 5:23am
v6
Comments
Tomas Takac 23-Nov-14 6:20am    
There is so much wrong with the code, I don't know where to start... i.ToCharArray() is unnecessary, you can access String the same way. Convert.ToInt32 all over the place: do it once and store the result in a local variable. Clearly you are looking for specific characters in the string. Have a look on Char.IsXXX methods, that may help.
Richard MacCutchan 23-Nov-14 6:21am    
This is a good example of really bad code. Why convert characters to integers to test their value, rather than just doing a simple character compare? For example the value 65 is x41 in hexadecimal which is the character 'A'. Whoever wrote this code needs a lot more training.
Andreas Gieriet 23-Nov-14 10:03am    
It's not just bad - to be more precise, it's over-engineered. I think it helps the OP more if we make clear distinction what bad means.
Over-engineering (AKA Code Bloat) is a common situation in software engineering. Most likely due to lack of peer review and lack of knowing the libraries well enough. See also my solution #3 and my comments for solution #1 and #2. I think the bashing does not help the OP to understand the issues and how to solve them.
Cheers
Andi
Richard MacCutchan 23-Nov-14 10:20am    
I explained why it was bad. And I guess you missed the original question which was much more than you now see.
Andreas Gieriet 23-Nov-14 10:37am    
I checked the revision history and simply see over-engineered code. It works, everyone could deduce what it does. It was consistently written - not so maintainable, I admit. Basically, it's a 1-liner if you use the right tools. Using the encoding is an overkill but not wrong. Using plain numbers is rather the difficult part since this assumes everyone knows the ASCII codes by heart. So, the request of commenting the code is not so odd - ok, line-by-line is a bit too much ;-). You told above what you would change, but below the "That's even worse" is not clear what to do with that statement. The OP is left alone.
Cheers
Andi

Do you have any idea how much work explaining code line by line is?
Every single line needs a paragraph of explanation! For example:
int next = r.Next();

Create a new variable called "next" which can hold a integer value. From the previously declared Random instance "r", call the "Next" method to get a new random number, and assign it to the "next" variable.

Can you imagine how long it would take us to explain even a very short code fragment like your example, line by line?

No. It is not going to happen. If you have a specific problem, then ask a question about it. But think first - would you want to sit down for 45 minutes and type up a line-by-line description for no good reason?
 
Share this answer
 
Comments
OriginalGriff 23-Nov-14 6:31am    
And what do *you* think it's doing?
And who wrote it, apart from a first year student with little idea of how to code?
Andreas Gieriet 23-Nov-14 8:35am    
My 5 for your reasoning that the request was inadequate.
Cheers
Andi
PS: The general bashing for the code being so bad is also not appropriate...
OriginalGriff 23-Nov-14 9:02am    
"The general bashing for the code being so bad is also not appropriate.."
I think it is.
You clearly haven't looked at the original code sample he wanted explained...

And it's pretty obvious he didn't write it - or he wouldn't have wanted it explained. If you were planning on using a "lump of unknown code" you found lying around, wouldn't *you* want to know it was rubbish? Before you invested any significant time in it? I would... ;)
Andreas Gieriet 23-Nov-14 9:31am    
I did not see the original code. The current state of the question is not that bad. It just bases on ASCII code instead of the character symbols. It is in that sense over-engineered and it uses plain numbers instead of constants (or even better the plain character). Still, I've seen far worse code than that. It is consistently coded, not easily maintainable but consistent. Everyone here has easily identified what it does - so, it cannot be that bad ;-).
Cheers
Andi
Andreas Gieriet 23-Nov-14 9:55am    
Ok, I checked now the original code. I'd say it is simply over-engineered, not wrong. Refactoring would be in several steps:
1) rename state to isIdentifier
2) remove char[] c = i.ToCharArray(); and replace all c[...] by i[...]
3) replace all Convert.ToInt32(i[...]) by i[...]
4) together with 3), replace the ASCII codes by the respective '...' symbols
5) make the for-loop from 1 instead of 0, since the first character is already checked
With these 5 simple transformations, you get a half way decent code. Now the question was if there were better ways than this transformed code? Yes, of course, but I think to start with, the transformed code shows at what to look at when getting such code...
Cheers
Andi
There is no particular reason for doing such thing. It's a nightmare :)
The (Convert.ToInt32(c[0]) >= 65 && Convert.ToInt32(c[0]) <= 90 || Convert.ToInt32(c[0]) == 95 || Convert.ToInt32(c[0]) >= 97 && Convert.ToInt32(c[0]) <= 122) code snippet is trying to check if the first character of a char array or a string is letter or underscore. But it is awful... :(
Such conversions are not needed at all. It would have been that simple: (c[0]>='A' && c[0]<='Z') || (c[0]>='a' && c[0]<='z') || c[0]=='_'... or even simpler with Char.IsLetter...

But as I see in the revision history, the rest of the code is also full of nonsense...

As the mehod's identifier is CheckIdentifier, it should do what it tells, but as C# for example is accepting unicode and digits in the identifiers, it is not C# identifier tester.
For C# it would be that simple:

C#
public static bool CheckIdentifier(String i) {
 CodeDomProvider provider = CodeDomProvider.CreateProvider("C#");
 return provider.IsValidIdentifier (i);
}
 
Share this answer
 
v2
Comments
Andreas Gieriet 23-Nov-14 9:44am    
Some thoughts: I try to avoid names like Check... unless they do some Assert class assertions in unit tests. I'd go for IsIdentifier(...) instead.
If the OP aims for C# identifiers is not obvious to me. E.g. a legal C# identifier is also @if, or i\u0064, etc. (see Escaping in C#: characters, strings, string formats, keywords, identifiers).
Cheers
Andi
I've seen much worse code than that - so, don't let you put down by some comments of some nice fellows... ;-)

Your request for a line-by-line explanation is beyond a QA - Quick Answer.

How can you analyze such code yourself in this case and in the future?
One possible approach:

  1. Read the documentation if there is one for the particular function block - do you understand the documented purpose of it? In reality, project documentation is scarce...
  2. Read the code documentation (comments of classes or functions): purpose, parameters, return value, remarks on constraints. If any code comment is to be up-to-date, then this is the one.
  3. Try to make sense of the class, function, parameter names. A decent coding gives reasonable names to the entities. Unfortunately, in reality, average quality code often lacks decent naming, e.g. int F1(int a1, int a2) { int temp = a1; ... }
  4. A lot of code I see is over-engineered: grown historically with insufficient knowledge of the straight and simplest solution for a given problem. Sharpen your eyes for such situations.
  5. Build up your "toolbox" of known patterns, idiomatic constructs, knowing the libraries and frameworks. In this case: what is the class Convert good for? You may come from a background where characters were checked by ASCII codes. This is at best the second best choice since it assumes you as a programmer know the character encoding of each character. Here, a symbolic approach (i.e. name the characters by their symbol and not by their encoding) was more adequate. An even better approach for checking text patterns is Regex (see suggested solution below).
  6. If you are asked to analyze and then refactor, create test cases first to cover the current functionality. Once you have a decent set of test cases that check for the required functionality, you are free to refactor the function at your discretion.


For the given case, it looks like you are checking some string if it matches some ASCII[^] encoded text pattern. I use Regex for such cases.
In your code, the plain numbers stand most likely for the ASCII codes as shown below:

Plain numberInterpreted as ASCII code
48'0'
57'9'
65'A'
90'Z'
95'_'
97'a'
122'z'


This function might be covered by the following:
C#
bool IsIdentifier(string strWord)
{
    return Regex.IsMatch(strWord ?? string.Empty, @"^[A-Za-z_][0-9A-Za-z_]*$");
}

Explanation:

  • the function IsMatch() checks if the input string matches the given pattern
  • gimmick: the first argument employs the coalescing operator ?? to take an empty string if the passed strWord argument was null
  • the second argument is the pattern to match
  • ^...$ says from begin (^) to end ($), i.e. the whole string, not only some part in between
  • [...] says that a given string character must match any of the characters listed between the [...]. Instead of listing all characters, you may give ranges of characters, e.g. [A-Za-z0-9_] means: any character from A...Z, a...z, 0...9 or _
  • * stands for zero to any number of instances. E.g. [a-z]* means: any number of instances of a...z characters.
  • The full pattern above reads: the given string must match as first character one of A...Z, a...z, _ then followed by any number of the characters A...Z, a...z, 0...9, _ to the end of the string.


Cheers
Andi
 
Share this answer
 
Comments
George Jonsson 23-Nov-14 10:46am    
A 5 for a good explanation
Andreas Gieriet 23-Nov-14 11:00am    
Thanks for your 5!
Cheers
Andi

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)



CodeProject, 20 Bay Street, 11th Floor Toronto, Ontario, Canada M5J 2N8 +1 (416) 849-8900