HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Two code snippets involving time-change

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
snippetsinvolvingtimetwocodechange

Problem

I have two snippets that are going to be used in a walkthrough and I'd like some feedback on which one is more easily understandable

1

property int hours, minutes, seconds
    property real shift: 0.0
    property bool night

    function timeChanged() {
        var date = new Date();
        hours = date.getUTCHours() + Math.floor(clock.shift);
        minutes = date.getUTCMinutes() + (clock.shift % 1) * 60;
        seconds = date.getUTCSeconds();
        night = (hours  19);
    }


2

property int hours, minutes, seconds
    property real shift
    property bool night

    function timeChanged() {
        var date = new Date;
        hours = shift ? date.getUTCHours() + Math.floor(clock.shift) : date.getHours()
        night = ( hours  19 );
        minutes = shift ? date.getUTCMinutes() + ((clock.shift % 1) * 60) : date.getMinutes()
        seconds = date.getUTCSeconds();
    }


I personally feel that instantiating the shift variable and skipping the ?: operator altogether is a better approach.

Note that both these snippets are aimed at a tutorial for people who are new to QML, and therefore I am charged with picking the one that seems simplest and clearest to people who don't know a lot about QML.

Solution

I definitely think sample #1 is more readable. The main reason is the (lack of) ternary operator. Although any programmer should understand it, it increases the amount of code you have to read. With sample #1 you can quickly skip the parts after + in the hours and minutes assignments, whereas with ? : you have to read more carefully.

Also, in sample #2 you basically add handling of a special case without needing it, adding unneeded complexity.

Another bonus in sample #1 is that night is not defined in the middle of the hours/minutes/seconds block, so the code is more logically grouped.

By the way, I think the use of shift should be documented with a comment.

Context

StackExchange Code Review Q#4013, answer score: 5

Revisions (0)

No revisions yet.