4
\$\begingroup\$

I have to create a quiz of showing questions from a JSON and post back the answers.

Here's the structure of JSON:

{
"pp": [
    {
        "profile_property": {
            "circle_value": 4,
            "created_at": null,
            "id": 1,
            "name": "THIS IS THE QUESTION TO DISPLAY",
            "updated_at": null,
            "profile_property_values": [
                {
                    "circle_value": 1,
                    "created_at": null,
                    "description": null,
                    "id": 1,
                    "profile_property_id": 1,
                    "standard": true,
                    "updated_at": "2014-01-24T13:34:47+05:30",
                    "value": "OPTION 1",
                    "image": "imageURL",
                    "sel_image": "selectedImageURL"
                },
                {
                    "circle_value": 2,
                    "created_at": null,
                    "description": null,
                    "id": 2,
                    "profile_property_id": 1,
                    "standard": true,
                    "updated_at": "2014-01-24T13:34:47+05:30",
                    "value": "OPTION @",
                    "image": "imageURL",
                    "sel_image": "selectedImageURL"
                },
                {
                    "circle_value": 3,
                    "created_at": null,
                    "description": null,
                    "id": 3,
                    "profile_property_id": 1,
                    "standard": true,
                    "updated_at": "2014-01-24T13:34:47+05:30",
                    "value": "OPTION #",
                    "image": "imageURL",
                    "sel_image": "selectedImageURL"
                }
            ]
        }
    },
    {
        "profile_property": {},
    {
        "profile_property": {},
    {
        "profile_property": {}
],
"style_profile": {
    }
  }
}

My Attempt

I have created a loaderVC which loads this JSON and creates model class. Then it pushes a quizVC which shows one question at a time. I have created a class which contains a static array to which I append the answers.

I know this design is very poor in many ways, so please suggest an object oriented approach to solve this.

Here are my classes:

loaderVC

class QuizLoaderViewController: UIViewController {

var quizData: Quiz? {
    didSet {
        showQuiz()
    }
}

override func viewDidLoad() {
    super.viewDidLoad()
    Answer.profileValueIds.removeAll(keepCapacity: true)
    self.navigationController!.navigationBarHidden = true
    getQuizQuestions()
}

func getQuizQuestions() {

    var urlString = Constants.baseUrl + "/quiz_questions.json"
    func onSuccess(jsonData: AnyObject) {

        let jsonResult = JSON_SWIFTY(jsonData)
        styleQuizData = StyleQuiz(fromJson: jsonResult)

    }
    func onFailure(error: NSError?) {
        SwiftSpinner.hide()
        println(error)

    }

    requestData(Method.GET, urlString, onSuccess, onFailure, self)

}

func showQuiz() {

    let storyboard = UIStoryboard(name: "Main", bundle: NSBundle.mainBundle())

    let quizVC = storyboard.instantiateViewControllerWithIdentifier("quiz") as! StyleQuizViewController
    quizVC.quizData = self.quizData
    styleQuizVC.index = 0
    self.navigationController!.pushViewController(quizVC, animated: false)
  }
}

quizVC

class QuizViewController: UIViewController,UICollectionViewDelegateFlowLayout,UICollectionViewDataSource {

@IBOutlet weak var collectionViewStyle: UICollectionView!

var quizData : Quiz?
var profileProperty:ProfileProperty?
var index = 0

override func viewDidLoad() {
    super.viewDidLoad()

    profileProperty = quizData?.pp[index].profileProperty
    topLabel.text = profileProperty?.name
    submitButton.hidden = true
    setupView()
}

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

@IBAction func closeButton(sender: UIButton) {
    self.navigationController?.popToRootViewControllerAnimated(true)
}

//MARK: Scroll View Delegate Methods

func numberOfSectionsInCollectionView(collectionView: UICollectionView) -> Int {
    return 1
}

func collectionView(collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
    return profileProperty?.profilePropertyValues.count ?? 0
}

func collectionView(collectionView: UICollectionView, cellForItemAtIndexPath indexPath: NSIndexPath) -> UICollectionViewCell {

    collectionView.registerNib(UINib(nibName: "StyleQuizValueCell", bundle: nil), forCellWithReuseIdentifier: "cellIdentifier")
    var cell = collectionView.dequeueReusableCellWithReuseIdentifier("cellIdentifier", forIndexPath: indexPath) as? StyleQuizValueCell

    let value = profileProperty!.profilePropertyValues[indexPath.row]

    cell?.imgView.image = UIImage(named: "preloader")
    let url = NSURL(string: value.image)
    cell!.imgView.hnk_setImageFromURL(url!)

    cell!.valueLabel.textColor = UIColor.whiteColor()
    cell!.valueLabel.text = value.value
    return cell!
}


func collectionView(collectionView: UICollectionView, didSelectItemAtIndexPath indexPath: NSIndexPath) {

    Answer.profileValueIds.append(profileProperty!.profilePropertyValues[indexPath.row].id)


    let cell = collectionView.cellForItemAtIndexPath(indexPath)! as! StyleQuizValueCell
    let value = profileProperty!.profilePropertyValues[indexPath.row]
    let url = NSURL(string: value.selImage)
    cell.imgView.hnk_setImageFromURL(url!)

    if index < styleQuizData!.pp.count - 1 {

        let storyboard = UIStoryboard(name: "Main", bundle: NSBundle.mainBundle())
        let styleQuizVC = storyboard.instantiateViewControllerWithIdentifier("styleQuiz") as! StyleQuizViewController
        styleQuizVC.styleQuizData = self.styleQuizData
        styleQuizVC.index = self.index+1
        self.navigationController!.pushViewController(styleQuizVC, animated: true)
    } else {
        submitButton.hidden = false
    }
}

@IBAction func goBack(sender: UIButton) {
    self.navigationController?.popViewControllerAnimated(true)
}

@IBAction func submitAction(sender: UIButton) {

    var urlString = Constants.baseUrl + "/style_profiles.json"
    let parameters = [
        "profile_property_value_ids": Answer.profileValueIds,
        "style_profile": ""
    ]
    func onSuccess(jsonData: AnyObject) {
        println(jsonData)
        var feedVC = self.navigationController?.viewControllers.first as! FeedViewController
        feedVC.url = "/home"
        // self.navigationController?.popToViewController(feedVC, animated: true)
        performSegueWithIdentifier("toHome", sender: self)
    }

    func onFailure(error: NSError?) {
        println(error)
    }

    requestData(Method.POST, urlString, onSuccess, onFailure, self, postParams: parameters)

  }
}

Answer struct

struct Answer { 
  static var profileValueIds = [Int](count: 10, repeatedValue : 10)
}

I am very new to object oriented programming. Please help me make a better design.

\$\endgroup\$
5
  • \$\begingroup\$ What's this mysterious requestData() function? \$\endgroup\$
    – nhgrif
    Commented Jun 5, 2015 at 23:18
  • \$\begingroup\$ requestData() makes async call to download the JSON \$\endgroup\$
    – saurabh
    Commented Jun 6, 2015 at 2:35
  • \$\begingroup\$ @nhgrif codereview.stackexchange.com/questions/91964/… \$\endgroup\$
    – saurabh
    Commented Jun 8, 2015 at 6:37
  • \$\begingroup\$ The JSON you have listed is not valid, I checked it here: jsonlint.com. Can you update it? \$\endgroup\$
    – Bryan Cimo
    Commented Jun 12, 2015 at 16:36
  • \$\begingroup\$ The JSON is not valid because I've trimmed similar 'profile_property' values to make it short and readable. \$\endgroup\$
    – saurabh
    Commented Jun 12, 2015 at 17:27

1 Answer 1

2
+50
\$\begingroup\$

It appears that your Answer struct is basically being used as a global singleton. It's hard to tell, because not everything is included here, but you've included your loaderVC, and the logic in that is hard to follow.

The loaderVC appears to exist purely for the sake of taking care of some of the loading logic. I'm not entirely certain how much I like that, but the problem is that there's a complete disconnect from loading the data and doing anything with the results.

Every time we set the styleQuizData property, we immediately segue. No matter what we set it to, even nil.

When there's an error loading the data, we just stop showing a spinner. Sure, we do a println, but the end user can't see that and we don't log that in a meaningful way that is useful once the app hits the app store. And from a user experience point of view, this is horrendous. These are the sort of apps that I just uninstall. When it breaks and doesn't tell me, but just sits there, I am not happy at all as an end user, so we need a better way of handling load errors (which won't be uncommon... you're doing networking...).

But if all we're doing in this loaderVC is simply loading the quiz data, it seems a pretty strong case for moving the loading logic to the previous view controller or to the next view controller. Yes, you will likely want some sort of temporary view to indicate that loading is happening, but not an entire view controller (which we get stuck on if the load fails).

Finally, if somehow, our network request completes before the view is finished appearing, we'll actually run into problems. You can't segue away from a view before it is finished appearing yet, and your code implements no logic to verify that loaderVC is ready to move forward before it presents another view controller. This is even more reason that our loading should be initialized from either our previous or next view controller rather than a view controller in the middle simply for the sake of loading the data.

\$\endgroup\$
4
  • \$\begingroup\$ Yes, the Answer is being used as global singleton \$\endgroup\$
    – saurabh
    Commented Jun 15, 2015 at 12:54
  • \$\begingroup\$ There's probably not a good reason for that. I wouldn't do this. \$\endgroup\$
    – nhgrif
    Commented Jun 15, 2015 at 12:56
  • \$\begingroup\$ Could you please suggest a better way to achieve the same? \$\endgroup\$
    – saurabh
    Commented Jun 15, 2015 at 13:29
  • \$\begingroup\$ That's a quite broad question, but here is a starting point: stackoverflow.com/questions/29734954/… \$\endgroup\$
    – nhgrif
    Commented Jun 15, 2015 at 13:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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