1
\$\begingroup\$

This works just as documented, but the code doesn't make it very clear that it does:

/**
 * Queues the given item and returns the success of this operation.
 * 
 * @param newItem      The item to enqueue
 * @param rowUUID      The optional UUID of an existing row.
 *
 * @return @c YES iff the given item could be enqueued.
 */
- (BOOL) addToQueue:(id)newItem
            rowUUID:(NSString *)rowUUID
{
    // This wrapper method allows use of the old method signature and be guaranteed the old method behavior.
    return [self addToQueue:newItem rowUUID:rowUUID updateOldRow:YES];
}

/**
 * Queues the given item and returns the success of this operation.
 * 
 * @param newItem      The item to enqueue
 * @param rowUUID      The UUID of an existing row if @c updateOldRow , else of a
 *                     new row.
 * @param updateOldRow If @c rowUUID is @c nil , does nothing. Else, if @c YES ,
 *                     requires a row with the UUID @c rowUUID to exist and updates
 *                     it with the new item. Else, creates a new row with that UUID.
 *
 * @return @c YES iff the given item could be enqueued.
 */
- (BOOL) addToQueue:(id)newItem
            rowUUID:(NSString *)rowUUID
       updateOldRow:(BOOL)updateOldRow
{
    // When updateOldRow == YES, the method behaves as it did before my changes.
    RowObject *row;
    if (rowUUID && updateOldRow)
    {
        row = [self getOldRowWithUUID:rowUUID];
        if (!row)
        {
            return NO;
        }
    }
    else
    {
        row = [self makeNewRow];
    }

    if (!rowUUID)
    {
        row.uuid = [self generateNewUUID];
    }
    else if (!updateOldRow)
    {
        row.uuid = rowUUID;
    }

    row.contents = newItem;

    return [self queueRow:row];
}

As the doc comment says, this is supposed to create or update a row in a queue with the given new item, based on the value of updateOldRow. If that is YES, then the method attempts to use rowUUID to pop an old row to update, but if it cannot find such a row, the method fails immediately. If updateOldRow is NO, then a new row is created with the given rowUUID, or a new one if none is given. After all that, the new item is saved in the row and the row is placed in the queue.


Please keep in mind, this is a genericized but working version of my code based on the real (proprietary) code; it behaves and is structured the same. Also, this has to be pored over to ensure it works just as documented, and even more-so if you don't trust/read the documentation.

How can I restructure this code so it works the same, but is easier to understand?

\$\endgroup\$
  • \$\begingroup\$ Obviously, this is a generic version of my code based on the real (proprietary) code Keep in mind example code is off-topic at Code Review and your question is definitely in a grey area. Please improve the question to lessen the risk of closure. \$\endgroup\$ – Mast Apr 22 '16 at 18:37
  • \$\begingroup\$ Ah, I see. My apologies, I didn't read the comments - usually reviewees describe what their code does in plain English, in the body of the question. \$\endgroup\$ – Mathieu Guindon Apr 22 '16 at 18:39
  • 1
    \$\begingroup\$ Let me make an edit to the post that I think will help it \$\endgroup\$ – Phrancis Apr 22 '16 at 19:46
  • 1
    \$\begingroup\$ Also keep in mind that we can review the code you posted, as is. We can't review the code that you decided not to post, so your disclaimer doesn't do much. \$\endgroup\$ – 200_success Apr 22 '16 at 21:05
  • 2
    \$\begingroup\$ The first step would to get rid of that updateOldRow parameter again and make it two separate methods. If there is a law that this method must keep it, make the distinction right at the top of the method body - and maybe put both branches into separate methods. \$\endgroup\$ – Eiko May 15 '16 at 14:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.