5
\$\begingroup\$

I have the following function in Swift and am looking to improve the readability. The function takes from the deviceData object which holds maps of data like [String:Double], [String:Int], and sometimes just regular String. The function is meant to format the data in the maps into Strings and return an array of Strings. Adding the variables to the Array below all of the let variables seems kind of funky to me and I was thinking maybe it would be good to declare an empty array above, get rid of the variable declarations and just add each value to the array but I'm not sure if that would be better or worse for performance. How can I improve the readability of this function without sacrificing performance?

func createFormattedDataStrings(deviceData: DeviceData, timeMilli: Double) -> Array<String?> { let formattedBmsVersion = (deviceData.bmsVersion?.first!.key)! + " \(deviceData.bmsVersion!.first!.value)" let formattedBoardVersion = (deviceData.boardVersion?.first!.key)! + " \(deviceData.boardVersion!.first!.value)" let formattedCellOne = (deviceData.cellOne?.first?.key)! + String(format: "%.2f", deviceData.cellOne!.first!.value) + "V" let formattedCellTwo = (deviceData.cellTwo?.first!.key)! + String(format: "%.2f", deviceData.cellTwo!.first!.value) + "V" let formattedCellThree = (deviceData.cellThree?.first!.key)! + String(format: "%.2f", deviceData.cellThree!.first!.value) + "V" let formattedCellFour = (deviceData.cellFour?.first!.key)! + String(format: "%.2f", deviceData.cellFour!.first!.value) + "V" let formattedCellFive = (deviceData.cellFive?.first!.key)! + String(format: "%.2f", deviceData.cellFive!.first!.value) + "V" let formattedCellSix = (deviceData.cellSix?.first!.key)! + String(format: "%.2f", deviceData.cellSix!.first!.value) + "V" let formattedCellSeven = (deviceData.cellSeven?.first!.key)! + String(format: "%.2f", deviceData.cellSeven!.first!.value) + "V" let formattedCellEight = (deviceData.cellEight?.first!.key)! + String(format: "%.2f", deviceData.cellEight!.first!.value) + "V" let formattedCellNine = (deviceData.cellNine?.first!.key)! + String(format: "%.2f", deviceData.cellNine!.first!.value) + "V" let formattedCellTen = (deviceData.cellTen?.first!.key)! + String(format: "%.2f", deviceData.cellTen!.first!.value) + "V" let formattedCellEleven = (deviceData.cellEleven?.first!.key)! + String(format: "%.2f", deviceData.cellEleven!.first!.value) + "V" let formattedCellTwelve = (deviceData.cellTwelve?.first!.key)! + String(format: "%.2f", deviceData.cellTwelve!.first!.value) + "V" let formattedCellThirteen = (deviceData.cellThirteen?.first!.key)! + String(format: "%.2f", deviceData.cellThirteen!.first!.value) + "V" let formattedCellFourteen = (deviceData.cellFourteen?.first!.key)! + String(format: "%.2f", deviceData.cellFourteen!.first!.value) + "V" let formattedPackVoltage = (deviceData.packVoltage?.first!.key)! + String(format: "%.1f", deviceData.packVoltage!.first!.value) + "V" let formattedPackSoc = (deviceData.packSoc?.first!.key)! + String(deviceData.packSoc!.first!.value) + "%" let formattedChargeTemp = (deviceData.chargeTemp?.first!.key)! + " \(deviceData.chargeTemp!.first!.value)°F" let formattedDischargeTemp = (deviceData.dischargeTemp?.first!.key)! + " \(deviceData.dischargeTemp!.first!.value)°F" let formattedChargeCurrent = (deviceData.chargeCurrent?.first!.key)! + " \(deviceData.chargeCurrent!.first!.value)A" let formattedChargeCircuitState = (deviceData.chargeCircuitState?.first!.key)! + " \(deviceData.chargeCircuitState!.first!.value)" let formattedDischargeCircuitState = (deviceData.dischargeCircuitState?.first!.key)! + " \(deviceData.dischargeCircuitState!.first!.value)" let formattedBalanceCircuitState = (deviceData.balanceCircuitState?.first!.key)! + " \(deviceData.balanceCircuitState!.first!.value)" let formattedEmptyCircuitState = (deviceData.emptyCircuitState?.first!.key)! + " \(deviceData.emptyCircuitState!.first!.value)" let formattedRestMode = deviceData.restMode let formattedMisbalancedMode = deviceData.misbalancedMode let formattedLeftChargedMode = deviceData.leftChargedMode let formattedHighVoltageMode = deviceData.highVoltageMode let formattedHighTempMode = deviceData.highTempMode let formattedLowTempMode = deviceData.lowTempMode let formattedLowVoltageMode = deviceData.lowVoltageMode let formattedHibernationMode = deviceData.hibernationMode let formattedOverDischargeMode = deviceData.overDischargeMode let formattedShipMode = deviceData.shipMode let formattedStateError = deviceData.stateError let formattedFinalCharge = deviceData.finalCharge let formattedFinalDischarge = deviceData.finalDischarge let formattedFinalBalance = deviceData.finalBalance let formattedFinalEmpty = deviceData.finalEmpty var dataStringArray: Array<String?> = [formattedBmsVersion, formattedBoardVersion, formattedCellOne, formattedCellTwo, formattedCellThree, formattedCellFour, formattedCellFive, formattedCellSix, formattedCellSeven, formattedCellEight, formattedCellNine, formattedCellTen, formattedCellEleven, formattedCellTwelve, formattedCellThirteen, formattedCellFourteen, formattedPackVoltage, formattedPackSoc, formattedChargeTemp, formattedDischargeTemp, formattedChargeCurrent, formattedChargeCircuitState, formattedDischargeCircuitState, formattedBalanceCircuitState, formattedEmptyCircuitState, formattedRestMode, formattedMisbalancedMode, formattedLeftChargedMode, formattedHighVoltageMode, formattedHighTempMode, formattedLowTempMode, formattedLowVoltageMode, formattedHibernationMode, formattedOverDischargeMode, formattedShipMode, formattedStateError, formattedFinalCharge, formattedFinalDischarge, formattedFinalBalance, formattedFinalEmpty] if (deviceData.bmsVersion?.first?.value)! >= 3634 { let formattedHotChargerMode = deviceData.hotChargerMode let formattedBadCharger = deviceData.badCharger dataStringArray.insert(formattedHotChargerMode!, at: 30) dataStringArray.insert(formattedBadCharger!, at: 41) } return dataStringArray }

Here is the deviceData specification:

import Foundation public struct DeviceData { var bmsVersion: [String:Int]? var boardVersion: [String:Double]? var cellOne: [String:Double]? var cellTwo: [String:Double]? var cellThree: [String:Double]? var cellFour: [String:Double]? var cellFive: [String:Double]? var cellSix: [String:Double]? var cellSeven: [String:Double]? var cellEight: [String:Double]? var cellNine: [String:Double]? var cellTen: [String:Double]? var cellEleven: [String:Double]? var cellTwelve: [String:Double]? var cellThirteen: [String:Double]? var cellFourteen: [String:Double]? var packVoltage: [String:Double]? var packSoc: [String:Int]? var chargeTemp: [String:Int]? var dischargeTemp: [String:Int]? var chargeCurrent: [String:Int]? var chargeCircuitState: [String:String]? var dischargeCircuitState: [String:String]? var balanceCircuitState: [String:String]? var emptyCircuitState: [String:String]? var restMode: String? var misbalancedMode: String? var leftChargedMode: String? var highVoltageMode: String? var highTempMode: String? var hotChargerMode: String? var lowTempMode: String? var lowVoltageMode: String? var hibernationMode: String? var overDischargeMode: String? var shipMode: String? var stateError: String? var finalCharge: String? var finalDischarge: String? var finalBalance: String? var finalEmpty: String? var badCharger: String? var dischargeShort: String? func getDeviceDataCount() -> Int { if (hotChargerMode == nil) { return 40 } else { return 43 } } } ```
\$\endgroup\$
10
  • \$\begingroup\$ Can you add the exact definition of DeviceData? Is there some specification of the data? \$\endgroup\$ Commented Aug 7, 2022 at 15:16
  • \$\begingroup\$ @MartinR I just added it. \$\endgroup\$ Commented Aug 8, 2022 at 17:09
  • \$\begingroup\$ I just wonder why all the members are dictionaries, and only one entry of each dictionary is used. Is that intentional? \$\endgroup\$ Commented Aug 8, 2022 at 18:25
  • \$\begingroup\$ @MartinR Yes, I intended to do that to accomplish what felt like easier data formatting for the display. I have data that I wanted to access and manipulate but I also had strings to describe that data so I just put them together with a data structure I was familiar with. Should I have used a different data structure? \$\endgroup\$ Commented Aug 8, 2022 at 19:06
  • \$\begingroup\$ Also, both members of the dictionary are being used in the formatting strings. \$\endgroup\$ Commented Aug 8, 2022 at 19:08

1 Answer 1

Reset to default
2
\$\begingroup\$

A commenter has asked me to actually review the code. I expect that comparing the OP's code to my solution provides its own critique, but sure I can get more explicit...

A review of the type itself:

  • Using a dictionary to represent a single String Value pair is problematic. Better to use a Tuple or a struct.
  • Having a number of properties that are manually numbered (e.g., cellOne, cellTwo, etc.) creates unnecessary duplication. Just use an array.
  • Using optional containers, String? in this case, is only valuable if there is a business case to be made between "empty" and "non-existent". I'm assuming there is for the context of this review, but I think it should be mentioned anyway.

A review of the function:

  • There is lot's of duplication here that can be simplified. Specifically there are a limited number of ways to format the data.
  • Moving this function into an extension of its first argument will dramatically limit the amount of boiler-plate code needed (the constant repetition of deviceData inside the function). Type extensions are a good way to establish scope when you are doing extensive work on a particular object/value.
  • A let assignment is great for when the right side of the operator is much more complex than the left side, but not as valuable in other cases. For example, let formattedRestMode = deviceData.restMode doesn't really add value... Just use deviceData.restMode directly.

Here's the simplification I came up with. In the future, I suggest you write a unit test so that you can mess with the guts of the function and know that your changes still work...

func formatted<T>(_ value: (String, T)?, format: (T) -> String = { "\($0)" }) -> String? { "\(value!.0) \(format(value!.1))" } extension DeviceData { func createFormattedDataStrings(timeMilli _: Double) -> [String?] { [ formatted(bmsVersion), formatted(boardVersion) ] + cells.map { formatted($0, format: { "\(String(format: "%.2f", $0))V" }) } + [ formatted(packVoltage, format: { "\(String(format: "%.1f", $0))V" }), formatted(packSoc, format: { "\($0)%" }), formatted(chargeTemp, format: { "\($0)°F" }), formatted(dischargeTemp, format: { "\($0)°F" }), formatted(chargeCurrent, format: { "\($0)A" }), formatted(chargeCircuitState), formatted(dischargeCircuitState), formatted(balanceCircuitState), formatted(emptyCircuitState), restMode, misbalancedMode, leftChargedMode, highVoltageMode, bmsVersion!.1 >= 3634 ? hotChargerMode : nil, highTempMode, lowTempMode, lowVoltageMode, hibernationMode, overDischargeMode, shipMode, stateError, finalCharge, finalDischarge, finalBalance, finalEmpty, bmsVersion!.1 >= 3634 ? badCharger : nil ] } } public struct DeviceData { var bmsVersion: (String, Int)? var boardVersion: (String, Double)? var cells: [(String, Double)] = [] var packVoltage: (String, Double)? var packSoc: (String, Int)? var chargeTemp: (String, Int)? var dischargeTemp: (String, Int)? var chargeCurrent: (String, Int)? var chargeCircuitState: (String, String)? var dischargeCircuitState: (String, String)? var balanceCircuitState: (String, String)? var emptyCircuitState: (String, String)? var restMode: String? var misbalancedMode: String? var leftChargedMode: String? var highVoltageMode: String? var highTempMode: String? var hotChargerMode: String? var lowTempMode: String? var lowVoltageMode: String? var hibernationMode: String? var overDischargeMode: String? var shipMode: String? var stateError: String? var finalCharge: String? var finalDischarge: String? var finalBalance: String? var finalEmpty: String? var badCharger: String? var dischargeShort: String? }
\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.