2
\$\begingroup\$

I got to show the createdAt date in a message timeline, I tried to make a separate array only for dates, but don't know if it is the right path to follow. It seems to me that my workaround is too tricky.

I'd like to be sure that what I've done is not necessarily the right way, but at least it isn't a wrong way (Edison's style). How can I optimize this date-related code?

I highlighted code I'm talking about with ****** problem x/3 *******

class MessagesTimelineTVC: UITableViewController {

    //    var myRefreshControl : UIRefreshControl!

    var timelineData : [String] = []
    var messageDateArray : [NSDate] = []

    func loadData() {

        timelineData.removeAll(keepCapacity: true)          //erase previus contents

        var findTimeLineDataQuery = PFQuery(className: "Messages")
        findTimeLineDataQuery.findObjectsInBackgroundWithBlock({
            (objects : [AnyObject]?, error : NSError?) -> Void in

            if error == nil {
                for singleObject in objects! {
                    if let stringData = singleObject["message"] as? String {
                        self.timelineData.append(stringData)
                        //MARK:                        *********** problem 1/3 *********
                        if let messageDate = singleObject.createdAt {
                            self.messageDateArray.append(messageDate!)
                            println(messageDate!)
                        }
                        //MARK:                        *********** problem  1/3 *********         
                    }
                }

                let reversedArray : Array = self.timelineData.reverse()  //MARK: Always Remember!
                self.timelineData = reversedArray as Array

                //MARK:                        *********** problem  2/3 *********
                let reversedMessageDateArray : Array = self.messageDateArray.reverse()  //MARK: Always Remember!
                self.messageDateArray = reversedMessageDateArray as Array
                //MARK:                        *********** problem  2/3 *********
                self.tableView.reloadData()
            }
        })
    }
    //    MARK: Parse
    override func viewDidAppear(animated: Bool) {

        self.loadData()

        if PFUser.currentUser() == nil {
            var loginAlertController = UIAlertController(title: "Sign up / login", message: "please sign up or login", preferredStyle: UIAlertControllerStyle.Alert)

            loginAlertController.addTextFieldWithConfigurationHandler({
                textfField in
                textfField.placeholder = "Your username"
            })

            loginAlertController.addTextFieldWithConfigurationHandler({
                textfField in
                textfField.placeholder = "Your password"
                textfField.secureTextEntry = true
            })

            //            MARK: login action in the array
            loginAlertController.addAction(UIAlertAction(title: "Login Action", style: UIAlertActionStyle.Default, handler: {
                alertAction in
                let textFields : NSArray = loginAlertController.textFields!
                let usernameTextField : UITextField = textFields[0] as! UITextField
                let passwordTextField : UITextField = textFields[1] as! UITextField     
                //MARK: Parse login problem - 15:39
                PFUser.logInWithUsernameInBackground(usernameTextField.text, password: passwordTextField.text){
                    (user: PFUser?, error: NSError?) -> Void in

                    if user != nil {
                        println("login success!")
                    } else {
                        println("login failed!")
                    }
                }
            }))

            //            MARK: sign up action in the array
            loginAlertController.addAction(UIAlertAction(title: "Sign up", style: UIAlertActionStyle.Default, handler: {
                alertAction in
                let textFields : NSArray = loginAlertController.textFields!
                let usernameTextField : UITextField = textFields[0] as! UITextField
                let passwordTextField : UITextField = textFields[1] as! UITextField

                var messageSender = PFUser() //16:42
                messageSender.username = usernameTextField.text
                messageSender.password = passwordTextField.text       
                messageSender.signUpInBackgroundWithBlock({
                    (success: Bool, error: NSError?) -> Void in
                    if error == nil {
                        println("sign up successful")
                    } else {
                        //                        let errorString = error!.userInfo["error"] as! String
                        let errorString = error!.localizedDescription
                        println(errorString)
                    }
                })

            })) 
            self.presentViewController(loginAlertController, animated: true, completion: nil)          
        }

    }

    override func viewWillAppear(animated: Bool) {

        //        self.loadData()   
        //        self.myRefreshControl = UIRefreshControl()
        //        self.myRefreshControl.attributedTitle = NSAttributedString(string: "Pull to refresh")
        //        self.myRefreshControl.addTarget(self, action: "refresh:", forControlEvents: UIControlEvents.ValueChanged)
        //        self.tableView.addSubview(myRefreshControl)

    }

    func refresh(sender:AnyObject)
    {
        //        var loaded = false

        //        self.loadData()
        //
        //        loaded = true
        //        if loaded {
        //        tableView.reloadData()
        //        self.myRefreshControl.endRefreshing()
        //        }

    }

    override func viewDidLoad() {
        super.viewDidLoad()

        //        self.loadData()   //if I call this here, messages will be doubled

        // Uncomment the following line to preserve selection between presentations
        // self.clearsSelectionOnViewWillAppear = false

        // Uncomment the following line to display an Edit button in the navigation bar for this view controller.
        // self.navigationItem.rightBarButtonItem = self.editButtonItem()
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
        // Dispose of any resources that can be recreated.
    }

    // MARK: - Table view data source

    override func numberOfSectionsInTableView(tableView: UITableView) -> Int {
        // #warning Potentially incomplete method implementation.
        // Return the number of sections.
        return 1
    }

    override func tableView(tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        // #warning Incomplete method implementation.
        // Return the number of rows in the section.
        return timelineData.count
    }

    //MARK: WARNING! Cast from 'String' to unrelated type 'PFObject' always fails
    override func tableView(tableView: UITableView, cellForRowAtIndexPath indexPath: NSIndexPath) -> UITableViewCell {

        let cell : MyCellTableViewCell = tableView.dequeueReusableCellWithIdentifier("myCellID", forIndexPath: indexPath) as! MyCellTableViewCell

        cell.messageCellLabel?.text = self.timelineData[indexPath.row]
        cell.nameCellLabel.text = "Sender Name and Row: \(indexPath.row)"

        //MARK:                        *********** problem  3/3 *********
        var dateFormatter = NSDateFormatter()
        dateFormatter.dateFormat = "MM-dd-yyyy HH:mm"
        var useDate = dateFormatter.stringFromDate(self.messageDateArray[indexPath.row])
        cell.dateMessageLabel.text = useDate
        //MARK:                        *********** problem  3/3 *********

        if indexPath.row % 2 == 0 {
            cell.backgroundColor = UIColor(red: 143 / 255, green: 204 / 255, blue: 255 / 255, alpha: 1)
            cell.messageCellLabel.backgroundColor = UIColor(red: 63 / 255, green: 159 / 255, blue: 255 / 255, alpha: 1)

        } else {
            cell.backgroundColor = UIColor(red: 63 / 255, green: 159 / 255, blue: 255 / 255, alpha: 1)
            cell.messageCellLabel.backgroundColor = UIColor(red: 143 / 255, green: 204 / 255, blue: 255 / 255, alpha: 1)
        }

        return cell
    }


    override func tableView(tableView: UITableView, didSelectRowAtIndexPath indexPath: NSIndexPath) {

        tableView.deselectRowAtIndexPath(indexPath, animated: true)
        println(indexPath.row)
    }
}
\$\endgroup\$
  • \$\begingroup\$ Does this work? If it works, you've done "not a wrong way". At Code Review, we're not interested in "not the wrong way"--we're only interested in "the right way". \$\endgroup\$ – nhgrif Aug 27 '15 at 12:21
  • \$\begingroup\$ Hi, yes it works, but I'm new at coding, still learning, so basically I'm asking for this kind of help: "please look at the code, am I forgetting something such multi threading matters, too many query inside pfquery". Isn't this a review? \$\endgroup\$ – biggreentree Aug 27 '15 at 12:27
5
\$\begingroup\$

You have left commented code in. There's no reason to leave commented code in--we should let source control handle that. If we don't need it, eliminate it completely, because huge chunks of commented code are simply clutter that some maintainer will eventually delete anyway. And we shouldn't leave in things that we know a maintainer will (and should) delete.


You've implemented a view life cycle event method (viewWillAppear(animated:)) without calling super. This is a no-no, and I'm only disappointed that Swift doesn't have a means for generating a warning for this like Objective-C does.


You have left in empty methods.

Empty methods come in three forms:

  • literally empty
  • no executable code in the method (only comments)
  • only executable code in the method is a call to super

You should never leave a purely empty method in our code. It only leads to confusion.

There is only one scenario in which it would be excusable to leave in a method that only calls to super, and that's a very Swift specific thing--only in an initializer. And that's because, as we know, Swift init inheritance is a little weird.

You've left the following offenders of this in your code:

  • viewWillAppear(animated:)
  • refresh(sender:)
  • viewDidLoad()
  • didReceiveMemoryWarning()

Also, it's less clear, but numberOfSectionsInTableView(tableView:) is also in violation of this, as it is an optional method for the protocol, and if it is not present, 1 (which you've hardcoded) is assumed.


You've left "incomplete implementation" warnings in your code:

// #warning Potentially incomplete method implementation.

and

// #warning Incomplete method implementation.

I don't remember seeing this in Swift, so I'm not sure if they generate actual warnings like the same boiler plate in Objective-C does, but either way, the comment is misleading. We're happy what the method does--it's no longer incomplete. Remove the comment.


UIColor(red: 63 / 255, green: 159 / 255, blue: 255 / 255, alpha: 1)

I've personally escalated in-line instantiation of colors like this to the same level of problem as magic numbers and magic strings. What does this color represent? What happens if we decide on a slightly different shade later (how many places do we have to go change)?

Much as we should prefer a set of constants over magic number literals and magic string literals, we should also prefer a set of constant colors for the colors we use throughout our app.

Colors.swift

struct Colors {
    static let SomeColorNameShade1 = UIColor(red: 63 / 255, green: 159 / 255, blue: 255 / 255, alpha: 1)
    static let SomeColorNameShade2 = UIColor(red: 143 / 255, green: 204 / 255, blue: 255 / 255, alpha: 1)
}

Now we use Colors.SomeColorNameShade1 throughout our app. If we ever need to change the shade, we change it in one spot.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you so much! I'm working on it! golden rules you teached will be immediately applied to all of my code! But now i have a logic problem here stackoverflow.com/questions/31777997/… on the same code, if you have the time, please take a look! \$\endgroup\$ – biggreentree Aug 30 '15 at 9:30

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.