Add a Logger class to centralize Log Management by kamalca · Pull Request #95 · ucladevx/Bruin-Bite-iOS

kamalca · 2018-09-01T23:26:07Z

All print statements now use a centralized class to report errors, warnings, and other information.

Note: Some messaged from pods or from UI elements still do not use the error class because I did not want to edit any of the locked files that are in charge of printing these messages.

I did my best to clean up the error reporting so that any error could be traced back to the code that produced them.

As for reporting what view controller they came from, the error itself should be good enough to track which code the error came from. However, something that I I looked into and may be useful is reporting which ViewController is open when the error was reported. This feature could be added upon request.

This error logger will be used to report all errors, warnings, info, etc. This is useful becasue it standardizes how information is reported to the output console and will allow error messages to be labled and ranked on importance.

All print statements in the project have been made to be more useful and more organized by using the Logger class.

HirdayGupta

Hey! Sorry for the delayed response, great job adding in the new class and calling it instead of all of our (overly generous) uses of the print statement! I think I agree with your ErrorType classifications on most of the print statements you replaced.

Leaving some requested changes that are mainly aesthetic in nature. Great work otherwise!

HirdayGupta · 2018-09-13T19:15:20Z

+}
+
+class Logger {
+    static let shared = Logger()


@kamalca @ayush-patel Do you foresee the Logger class ever having a state of its own? (I.e. its own private data). It doesn't right now, and if that isn't something that we foresee, then we can change this from a Singleton class to just a class with static functions. (UPDATE: I don't think its ever going to have a state of its own in the foreseeable future, so go with the static approach please)

That change would involve changing all the (one) functions to static and removing the static let shared singleton (because all function calls would be through a static function).

This means that all calls to this class will now look like this: Logger.handle(type: message:)

HirdayGupta · 2018-09-13T19:37:55Z

+
+    private init() {    }
+
+    func handle(type:ErrorType, message:String) {


Suggest changing the method name & using function argument labels to modify the function signature to the following: (and using static to abide by the previous suggestion)

static func log(message: String, ofType type: ErrorType)

This and the switch from singleton to static would mean that calls to this function would now look like:

Logger.log(message: "Hi! I'm an error!", ofType: .error)

An alternate is dropping the label for message entirely, so the function definition would look like this:

static func log(_ message: String, withType type: ErrorType)

Now, this change and the switch from singleton to static would mean that calls to this function would now look like:

Logger.log("Hi! I'm an error!", withType: .error)

I'm in favor of the second one because the function name log, implies the verb, and hence, the label message for the first argument is kind of implied, because what else would you be logging lol. (EDIT: @kamalca let me know which alternative you prefer, and if you agree with the second one, go ahead and implement it!)

Side note: This may seem trivial, but think about it this way - the interface you are designing now is going to be used across the app for many more versions to come. Plus, our code could do with some beauty 😄

HirdayGupta · 2018-09-13T19:44:17Z

@@ -0,0 +1,37 @@
+//
+//  ErrorLogger.swift


Rename the file to LogManager.swift and move it from the Models folder to a new folder called Managers.

HirdayGupta · 2018-09-13T19:45:38Z

+
+import Foundation
+
+enum ErrorType: String {


Rename this to LogLevel (because names like ErrorType should be reserved for our Centralised Error Handling mechanism, whenever we get to it).

When you do make this change, you should also replace the word 'type' from the argument name and labels in the handle function to 'level'.

HirdayGupta · 2018-09-13T19:46:16Z

+
+    func handle(type:ErrorType, message:String) {
+
+        if(message == "")


change to message.isEmpty

HirdayGupta · 2018-09-13T20:21:42Z

                    let results = try JSONDecoder().decode(MatchRequestData.self, from: response.data)
                    let jsonData = JSON(response.data)
-                    print(jsonData)
+                    Logger.shared.handle(type: .debug, message: "matchUser JSON data: \(jsonData)")


Good precedent for the .debug type!

HirdayGupta · 2018-09-13T20:22:00Z

+                    Logger.shared.handle(type: .error, message: "matchUser: \(err)")
                }
-                print("Successfully Sent match")
+                Logger.shared.handle(type: .verbose, message: "Successfully Sent match")


Change to .info

HirdayGupta · 2018-09-13T20:26:25Z

            self.topBar.timeSelected("Night")
        default:
-            print("Current Meal Period could not be determined")
+            Logger.shared.handle(type: .error, message: "Current Meal Period could not be determined")


Can you reconfirm if this needs to be .error or if we can change it to .warning (read the aforementioned descriptions for the intention of each type, basically, if the meal period not being determined breaks things in a very noticeable fashion then leave it as .error but if not, and its a minor inconvenience, then change to warning)

I believe this wold result in the behavior we were seeing last year where the menu would not load until you clicked on a different time. However, this switch statement covers all possible values of hour from zero (inclusive) to 24 (exclusive). Therefore the error is more of a formality. I can switch it to warning though.

HirdayGupta · 2018-09-13T20:26:59Z

        }
    } else {
-        print("No meal period selected")
+        Logger.shared.handle(type: .warning, message: "No meal period selected")


Confirm if this needs to be .error or can stay as a .warning

This warning/error occurs when someone tries to select a time before selecting a meal period. The resulting behavior is simply a popup that is not populated with times. I would not consider it a fatal error by any means. I suggest keeping it a warning and later alerting the user to this error when they attempt to put in a time before the meal period.

HirdayGupta · 2018-09-13T20:30:41Z

@kamalca When there are many requested changes, Github hides some of the requested changes by default, so be sure to click the show hidden button to get every requested change! Also feel free to comment on any of the review comments to start a discussion about the changes requested if you feel you didn't understand a requested change or do not agree.

@HirdayGupta

Carried out the changes requested by @HirdayGupta -- • Static functions instead of a singleton • Changed parameter names and function name from handle to log • Changed file from ErrorLogger to LogManager • Changed enumeration name from ErrorType to LogLevel • Removed raw values from enumeration and added a function to determine the header • Added documentation to enumeration • Revised some log messages and log levels

HirdayGupta · 2018-09-18T21:24:15Z

-        print("Device Token: \(token)")
-        print(token)
+        Logger.log("Device Token: \(token)", withLevel: .debug)
+        Logger.log(token, withLevel: .debug)


Should be safe to remove Line 90 because its a repeat.

HirdayGupta · 2018-09-18T21:37:42Z

+
+class Logger {
+
+    private init() {    }


Can remove the private initializer because an initialized Logger class has no use if all its functions are static (so even if a dev was to initialize a Logger object, it would be useless).

HirdayGupta

Whoops, left this out of my previous review.

HirdayGupta · 2018-09-28T17:33:29Z

@kamalca Any progress on this Pull Request?

made requested changes

HirdayGupta

Looks good, thanks for putting in the effort @kamalca! 😄

kamalca added 2 commits September 1, 2018 16:18

Wrote ErrorLogger class

5855e05

This error logger will be used to report all errors, warnings, info, etc. This is useful becasue it standardizes how information is reported to the output console and will allow error messages to be labled and ranked on importance.

Changed print statements to use the new Logging class

a99249d

All print statements in the project have been made to be more useful and more organized by using the Logger class.

kamalca requested a review from ayush-patel September 1, 2018 23:26

HirdayGupta requested changes Sep 13, 2018

View reviewed changes

HirdayGupta changed the title ~~Kameron error handling class~~ Add a Logger class to centralize Log Management Sep 13, 2018

HirdayGupta requested review from ayush-patel and removed request for ayush-patel September 13, 2018 20:51

HirdayGupta self-assigned this Sep 13, 2018

kamalca requested a review from HirdayGupta September 18, 2018 20:19

HirdayGupta requested changes Sep 18, 2018

View reviewed changes

Comment thread Bruin Bite/Managers/LogManager.swift

kamalca added 2 commits September 28, 2018 19:44

Merge branch 'development' into kameron_error_handling_class

7360794

Completed requested changes pt.2

85290ff

made requested changes

kamalca requested review from HirdayGupta and removed request for ayush-patel September 29, 2018 02:56

HirdayGupta approved these changes Oct 2, 2018

View reviewed changes

HirdayGupta added the s.onHold label Apr 27, 2019


		private init() { }

		func handle(type:ErrorType, message:String) {


		func handle(type:ErrorType, message:String) {

		if(message == "")

Conversation

kamalca commented Sep 1, 2018 • edited by HirdayGupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HirdayGupta left a comment

Choose a reason for hiding this comment

Uh oh!

HirdayGupta Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirdayGupta Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirdayGupta Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirdayGupta commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirdayGupta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HirdayGupta commented Sep 28, 2018

Uh oh!

HirdayGupta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kamalca commented Sep 1, 2018 •

edited by HirdayGupta

Loading

HirdayGupta Sep 13, 2018 •

edited

Loading

HirdayGupta Sep 13, 2018 •

edited

Loading

HirdayGupta Sep 13, 2018 •

edited

Loading