Return to Answer

added 2 characters in body

edited Oct 23, 2017 at 14:42

Flater

• 5.7k
• 18
• 23

Inline Side-by-side Side-by-side Markdown

added 316 characters in body

edited Oct 23, 2017 at 14:28

Flater

• 5.7k
• 18
• 23

Inline Side-by-side Side-by-side Markdown

answered Oct 23, 2017 at 14:22

Flater

• 5.7k
• 18
• 23

1. Don't reinvent the wheel.

---

This StackOverflow answer tackles the same problem that you've tackled. There are slight differences (e.g. no prefix parameter), but the linked example is immensely more readable and maintainable.

When you're trying to tackle a problem that sounds rather generic, always look for examples online. More often than not, someone else will have already been tackling this problem.

You mentioned that you approached this problem with the intention of learning how en/decoding works. Doing things yourself is a great way to learn; and I would indeed suggest that you trying doing something yourself instead of immediately relying on external help.

However, in your pursuit of trying to do it yourself, you dug much too deep. When you started using unmanaged code and pointers, that should've been a red flag that you were overcomplicating your solution.

You didn't really lose anything here, because it was a training exercise. But in a professional setting, your complicated solution must have taken a lot more time, while also lowering readability and making the code less maintainable in the future. And this is where your good intentions ended up doing more harm than good.

2. Pointers lower readability --

I'm not 100% sure if this unilaterally applies to C#. I think it does, but maybe I'm forgetting some use cases here. However, I am at least certain that your case did not need pointers.

Relative to other "building blocks" of programming, pointers are very difficult to grasp in any sufficiently complex codebase (= most professional codebases). My suggestion is to avoid them whenever possible.
While pointers are of course usable from a technical point of view, they generally detract from the readability of the code. Unless you're facing a fringe case (string operations are not fringe cases), the unreadability of pointers tends to outweigh the benefits of using them.

An example of an unnecessary pointer is target in GetString(). There are other ways of solving the problem that do not require a pointer.

var result = new string('☠', ((count << 1) + prefix.Length));
fixed (byte* source = &value[0])
fixed (char* target = result) {
 for (var i = 0; i < prefix.Length; i++) {
 *(target + i) = prefix[i];
 }
//...
while (0 < count--) {
 *t = m[*s];
 s++;
 t++;
} 

Assigning a string character by character is a cumbersome process. It can be performant (since you're working in unmanaged code), but it's ugly and prone to lowering readability.

Compare this to an approach that is more idiomatic (for C#):

StringBuilder sb = new StringBuilder();
sb.Append(prefix);
foreach(char c in value)
{
 byte b = Convert.ToByte(c);
 sb.AppendFormat("{0:x2}", b);
}
return sb.ToString();

Note: This is based on the conversion method from the linked answer (cfr the first part of my answer)

The only two variables I needed were value and prefix. Everything else could have been omitted.

The majority of your code focuses on handling pointers instead of focusing on your main goal (byte to string conversion).

If a car mechanic spends more time looking for his tools instead of fixing cars, wouldn't you agree that this mechanic is not working efficiently?

3. Bitshifting lowers readability. --

   var result = new string('☠', ((count << 1) + prefix.Length));

(count << 1) is the same as count * 2. However, in the interest of readability, you should favor using count * 2.

According to this answer, the compiler will evaluate whether a given multiplication (e.g. count * 2) can easily be solved using shift operations.

This means that when you use count * 2, that the code has become more readable, and the runtime performance will be the same (since the compiler will still do a bitshift if it's more performant).

Also note:
Bitshifting only works if you wish to multiply by a power of 2, because bits are binary (base 2). This wouldn't work for multiplication by any other number, which makes bitshifting a bad fit for multiplication.

4. You need to invest time into readability.

---

This chapter is a quick-fire round. I want to highlight a few snippets that are disproportionately unreadable.

1. Don't rely on magic numbers

private static readonly uint[] m_byteToHexMap = Enumerable
 .Range(0, 256)
 .Select(i => (((uint)(GetChar((byte)(i & 0b1111)) << 16)) | GetChar((byte)(i >> 4))))
 .ToArray();

Specifically:

(((uint)(GetChar((byte)(i & 0b1111)) << 16)) | GetChar((byte)(i >> 4)))

Even after furious Googling, I have no idea how this works. What's 0b1111? Why << 16? Why >> 4? It's pure magic.

Compare this to Convert.ToByte(c). The intention of this tiny snippet is immediately obvious, without relying on a ton of bit-level operations that would require a reader to intensely study the effects of bitshifting and the ASCII value of characters.

2. The intention/purpose of a variable should be understood by its name alone.

There are times where you stuck to this rule. E.g. the name of your prefix variable makes it immediately obvious that this string value will be added to the front of the output. The name aptly describes the purpose of the variable.

But then you also do this:

while (0 < count--) {
 *t = m[*s];
 s++;
 t++;
}

What is s? What is t? What is m?

Every step of the way, I've had to scroll up to find the declared variable. I've had to look up the same variable several times because I kept forgetting. The fact that these are pointers further compounds the complexity.

while (0 < count--) {
 *pTarget = mapping[*pSource];
 pSource++;
 pTarget++;
}

Forgoing the argument about not using pointers, the readability has already dramatically improved simply by giving the variables a name that aptly describes the variable and its purpose.

Note: You can or course make exceptions for index variables such as i. Index variables are a common occurrence, most developers will immediately assume that this is an integer that will be used to access an array.

5. Optional parameters are a C# feature.

---

You're not using optional variables for variables that are actually optional:

public static unsafe string GetString(byte[] value, string prefix) 
{
 prefix = (prefix ?? string.Empty);
 //...

If you have no prefix, then you would call the method using:

GetString(myValue, null);

However, if you had used an optional parameter:

public static unsafe string GetString(byte[] value, string prefix = "") 
{

Then you wouldn't have needed to check add the extra check to see if it is null. If it were null, it will automatically use the supplied value (""). This means that optional parameters will never be null (unless, of course, you make their default value null...)

This also means that you can omit optional parameters if you don't need them. If you have no prefix, then you would call the method using:

GetString(myValue);

Notice how you're not forced to add the null.

6. Your priorities.

---

This is a bit off-topic, more a review of your approach to coding and not the code itself.

Correctness of implementation is the most important thing IMO, performance second, and readability third

1. Correctness of implementation
2. Performance
3. Readability

We can argue about the order of priorities. I think most people would put readability over performance; because readability affects programmer performance, whereas performance affects machine performance.
More often than not, the speed of development is a bigger bottleneck than the processing speed by the server, which makes readability more important than code performance.

However, this is not what I want to talk about now. Instead, I want to pull focus to the fact that priorities should not be absolute. Not every performance boost will always trump a consideration of readability.

For the sake of (silly and extreme) example, let's say that you can get a 0.05% performance gain if all your method and variable names exclusively use Chinese characters. You don't speak Chinese, nor do your colleagues.

Would you use Chinese characters in all your methods?

A minuscule performance gain is not worth it when it comes at the cost of making the code twice as hard to read. Even though you put performance before readability, you should still weigh your options.

Just because you favor performance over readability, does not mean that you should always sacrifice readability in favor of performance. Instead, look at how much you stand to gain performance wise, and compare it to the cost of lowering readability.
If you can get a massive performance gain for a minor dent in eadability, go for it. If the performance gain is much smaller than the cost of losing readability, don't do it.

I can't really give you a definitive answer here, it's very subjective. But you need to observe the cost and benefit of a cetain change, rather than blindly following the priorities you defined in the past.