Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old April 23, 2016, 10:59   #11
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 54
Posts: 7,849
Donated: $60
Nick will become famous soon enough
I am in an interesting position with respect to this discussion. On one hand, I'm a latecomer to C programming, and don't have very strong feelings about style - I have in fact learnt a lot of my habits from first Oangband and now V.

On the other hand, in recent times I've probably been the person writing the most V code, so I'm the most directly affected by a change to coding style.

Given that, I reckon the rest of you (and anyone else who wants to get involved) should slug it out here, and report back to me when you're done. How does that sound?
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old April 23, 2016, 21:10   #12
molybdenum
Apprentice
 
Join Date: May 2013
Posts: 84
molybdenum is on a distinguished road
Quote:
Originally Posted by calris View Post
I don't understand - how does it 'collapse down'?
Sorry, I was referring to code folding; I use it all the time. I was trying to give an example of why I prefer to have a line break between the } and the else. Otherwise, I totally agree with what you are saying.
molybdenum is offline   Reply With Quote
Old April 23, 2016, 23:40   #13
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 8,929
Derakon is on a distinguished road
IMO control flow keywords should generally be the first non-whitespace on their line, with the possible exception of a do-while loop.
Derakon is online now   Reply With Quote
Old April 24, 2016, 04:14   #14
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by Derakon View Post
IMO control flow keywords should generally be the first non-whitespace on their line, with the possible exception of a do-while loop.
Here's a simple example of all the variants for the sake of comparisons

Style A - I think we can all agree this one is bad
Code:
    if (test_1) i++;
    else if (test_2) j++;
    else k++;
Style B (Linux Style) - Not as bad, but prone to introduce new bugs and adding a single statement to any line creates a patch which involves adding braces all over the place
Code:
    if (test_1)
        i++;
    else if (test_2)
        j++;
    else
        k++;
Style C: Linux Style + braces. Much safer than B. Adds one line the entire block over B (will only add one line no matter how many 'else' statements)
Code:
    if (test_1) {
        i++;
    } else if (test_2) {
        j++;
    } else {
        k++;
    }
Style D: Conditional statements on new line - Adds one line per conditional statement over B
Code:
    if (test_1) {
        i++;
    }
    else if (test_2) {
        j++;
    }
    else {
        k++;
    }
My preference is C - It is a good compromise between code safety and code density. I'm also biased towards C because that is the style I have used the most, so it feels 'natural' to me
calris is offline   Reply With Quote
Old April 24, 2016, 04:20   #15
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 8,929
Derakon is on a distinguished road
Yeah, my preference from those is D; I just find it easier to read, and C feels needlessly terse.
Derakon is online now   Reply With Quote
Old April 24, 2016, 04:27   #16
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by molybdenum View Post
Sorry, I was referring to code folding; I use it all the time. I was trying to give an example of why I prefer to have a line break between the } and the else. Otherwise, I totally agree with what you are saying.
I you need to fold code, there is deeper problem. I'm a personal fan of Linux's 'the length of a function should be inversely proportional to it's complexity' rule:

Quote:
The maximum length of a function is inversely proportional to the
complexity and indentation level of that function. So, if you have a
conceptually simple function that is just one long (but simple)
case-statement, where you have to do lots of small things for a lot of
different cases, it's OK to have a longer function.

However, if you have a complex function, and you suspect that a
less-than-gifted first-year high-school student might not even
understand what the function is all about, you should adhere to the
maximum limits all the more closely. Use helper functions with
descriptive names (you can ask the compiler to in-line them if you think
it's performance-critical, and it will probably do a better job of it
than you would have done).
There a a lot of overly complex functions in Angband with duplicated snippets of code all over the place.
  • Complex functions being longer that a screen long are difficult for mere mortals to process - Anything longer than 40 lines (be it a paragraph of text, a C function, a part of mathematical proof) pushes the limits of human memory and comprehension
  • Long, complex functions are daunting for new programs and create a barrier for entry for new coders that want to get involved. Keeping the barrier to entry for new coders low is 'A Good Thing'(tm)
  • Duplicated code snippets are prone to introduce bugs if changes are made that are not captured in ALL versions of the snippet
  • The code ends up having insane levels of indent, which pushes the 80 column limit to the max

The entire Linux kernel, nearly 17 million lines of code, is (mostly) less than 80 columns using 8 space tabs. Angband has a hard time using 4 space tabs.
calris is offline   Reply With Quote
Old April 24, 2016, 05:53   #17
molybdenum
Apprentice
 
Join Date: May 2013
Posts: 84
molybdenum is on a distinguished road
Quote:
Originally Posted by calris View Post
I you need to fold code, there is deeper problem.
I'd say folding is more of a visual preference, hiding stuff I don't want to look at or constantly scroll past.


Quote:
Originally Posted by calris View Post
I'm a personal fan of Linux's 'the length of a function should be inversely proportional to it's complexity' rule:

...

There a a lot of overly complex functions in Angband with duplicated snippets of code all over the place.

...

The entire Linux kernel, nearly 17 million lines of code, is (mostly) less than 80 columns using 8 space tabs. Angband has a hard time using 4 space tabs.
This is new concept for me, using certain aspects of code style to enforce simpler implementation. For example, restricting line length to 80 drives me nuts, but that's probably because I work with code that's particularly verbose. Just today I was having to throw around this constant: NSURLSessionAuthChallengeCancelAuthenticationChall enge (54 characters, and even the forum doesn't like that length). Just what I'm used to, I guess.
molybdenum is offline   Reply With Quote
Old April 24, 2016, 06:59   #18
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by molybdenum View Post
I'd say folding is more of a visual preference, hiding stuff I don't want to look at or constantly scroll past.
I'll happily fold functions. But even this can suggest that the source file is becoming too long and the function deserves to be somewhere else. main-win.c is a classic example - over 5000 lines - sound is less that 200 lines buried right in the middle

Quote:
This is new concept for me, using certain aspects of code style to enforce simpler implementation. For example, restricting line length to 80 drives me nuts
It used to drive me nuts too - But once you get into a mindset of 'my function needs to fit inside 80 columns and 30 lines' you develop a knack for taking what would otherwise been an insanely complicated concept to code and break it down into logical chunks.

Quote:
but that's probably because I work with code that's particularly verbose. Just today I was having to throw around this constant: NSURLSessionAuthChallengeCancelAuthenticationChall enge (54 characters, and even the forum doesn't like that length). Just what I'm used to, I guess.
That is just EVIL - compare these
  • NSURLSessionAuthChallengeCancelAuthenticationChall enge
  • NSURLSessionAuthChallengeAcceptAuthenticationChall enge

Having to focus on a tiny spec in the middle of two otherwise identical names is just asking for trouble - no wonder so much security related code is buggy
calris is offline   Reply With Quote
Old April 24, 2016, 17:22   #19
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 5,395
Donated: $40
Pete Mack is on a distinguished road
While I agree deeply nested code is bad, it's a long way from being the worst problem in angband. The worst problems are the ones that make modification a nightmare
1. Duplicated code.
2. Code related to a concept scattered across creation.
The latter lead to untold numbers of failed assertions in the known-objects change. I saw yet another one posted a couple days ago.
Pete Mack is offline   Reply With Quote
Old April 25, 2016, 07:13   #20
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
If pushed a draft version of the coding guidelines into my repo here

Comments are appreciated
calris is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Sil: 'forge' key in Angband-style keyset? Therem Harth Sil 3 October 24, 2012 18:29
Skyrim, angband style BlackKite Idle chatter 8 June 27, 2012 03:28
some more questions about coding Malak Darkhunter Vanilla 16 January 3, 2012 05:52
more variant coding woes will_asher Variants 1 February 10, 2011 06:18
Angband coding question will_asher Variants 10 September 3, 2008 23:03


All times are GMT +1. The time now is 19:02.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2019, vBulletin Solutions Inc.