So I have the following Image where I can utilizing two switch statements, so I wanted to see if there is a way for me to improve the performance, or trim down the functions into a single function, I have never done this before, so it will be a learning experience.
So I have the function:
@ViewBuilder func viewForPlace(_ place: Place) -> some View {
Image("map-pin")
.renderingMode(.template)
.resizable()
.frame(width: 35, height: 35)
.foregroundColor(place.show ? .red : Color.init(UIColor(hexString: mapAnnotationColor(place.category))))
.onTapGesture {
if !place.show {
tapOnMapAnnotation(
place: place
)
}
}
.overlay(
Image(mapAnnotationIcon(place.category)).renderingMode(.template).resizable().frame(width: 10, height: 8).foregroundColor(.white).padding(.bottom, 15)
)
}
It utilizes the following switch statements:
// MARK: - SET MAP ANNOTATION COLOR
private func mapAnnotationColor(_ category: String) -> String {
switch category {
case "Route":
return "#6A798E"
case "Scenic Area":
return "#006994"
case "Attraction":
return "#00954C"
case "Hospitality":
return "#6D5B97"
case "Landmark":
return "#5D5639"
default:
return "#FFFFFF"
}
}
// MARK: - SET MAP ANNOTATION ICON
private func mapAnnotationIcon(_ category: String) -> String {
switch category {
case "Route":
return "car-side"
case "Scenic Area":
return "camera"
case "Attraction":
return "people-group"
case "Hospitality":
return "bed"
case "Landmark":
return "landmark-flag"
default:
return "NA"
}
}
Is there a clear way to write this code where I don't have to utilize 2 switch statements or is it possible to have everything under one function?
CodePudding user response：
Define the necessary values upfront and then use them later.
@ViewBuilder func view(place: Place) -> some View {
let mapAnnotation = MapAnnotation(category: place.category)
Image("map-pin")
.renderingMode(.template)
.resizable()
.frame(width: 35, height: 35)
.foregroundColor(place.show ? .red : mapAnnotation.color)
.onTapGesture {
if !place.show {
tapOnMapAnnotation(
place: place
)
}
}
.overlay(
mapAnnotation.image.renderingMode(.template).resizable().frame(width: 10, height: 8).foregroundColor(.white).padding(.bottom, 15)
)
}
struct MapAnnotation {
init(category: Place.Category?) {
switch category {
case .route:
self.init(pixelArtColorFromThe1990s: "#6A798E", imageName: "car-side")
case .scenicArea:
self.init(pixelArtColorFromThe1990s: "#006994", imageName: "camera")
case .attraction:
self.init(pixelArtColorFromThe1990s: "#00954C", imageName: "people-group")
case .hospitality:
self.init(pixelArtColorFromThe1990s: "#6D5B97", imageName: "bed")
case .landmark:
self.init(pixelArtColorFromThe1990s: "#5D5639", imageName: "landmark-flag")
case nil:
self.init(pixelArtColorFromThe1990s: "FFFFFF", imageName: "NA")
}
}
let image: Image
let color: Color
private init(
pixelArtColorFromThe1990s: String,
imageName: String
) {
color = .init(UIColor(hexString: pixelArtColorFromThe1990s))
image = .init(imageName)
}
}
struct Place {
…
let category: Category?
enum Category {
case route, scenicArea, attraction, hospitality, landmark
}
}
CodePudding user response：
Of course, they can be combined into a single function:
private func mapAnnotationAttributes(_ category: String) -> (String, String)
{
switch category {
case "Route" : return ("car-side", "#6A798E")
case "Scenic Area": return ("camera", "#006994")
case "Attraction" : return ("people-group", "#00954C")
case "Hospitality": return ("bed", "#6D5B97")
case "Landmark" : return ("landmark-flag", "#5D5639")
default: return ("NA", "#FFFFFF")
}
}
The issue is that you use the values in two different methods on Image, so you'll need call mapAnnotationAttribtes prior so you have the results to pass to those methods.
@ViewBuilder func viewForPlace(_ place: Place) -> some View {
let (annotationIconName, annotationRGBString) = mapAnnotationAttributes(place.category)
return Image("map-pin")
.renderingMode(.template)
.resizable()
.frame(width: 35, height: 35)
.foregroundColor(place.show ? .red : Color.init(UIColor(hexString: annotationRGBString)))
.onTapGesture {
if !place.show {
tapOnMapAnnotation(
place: place
)
}
}
.overlay(
Image(annotationIconName).renderingMode(.template).resizable().frame(width: 10, height: 8).foregroundColor(.white).padding(.bottom, 15)
)
}
It occurs to me though that, at least in the given code, you don't actually need the Strings. Maybe you need them somewhere else for some reason, but in the code you provided, what you need is an icon Image and a Color. In my opinion your view viewForPlace code would be more readable if you just returned those from mapAnnotationAttribute.
private func mapAnnotationAttributes(_ category: String) -> (Image, Color)
{
let attribs: (String, String)
switch category {
case "Route" : attribs = ("car-side", "#6A798E")
case "Scenic Area": attribs = ("camera", "#006994")
case "Attraction" : attribs = ("people-group", "#00954C")
case "Hospitality": attribs = ("bed", "#6D5B97")
case "Landmark" : attribs = ("landmark-flag", "#5D5639")
default: attribs = ("NA", "#FFFFFF")
}
let icon = Image(attribs.0)
let color = Color.init(UIColor(hexString: attribs.1))
return (icon, color)
}
Then viewForPlace becomes:
@ViewBuilder func viewForPlace(_ place: Place) -> some View {
let (annotationIcon, annotationColor) = mapAnnotationAttributes(place.category)
return Image("map-pin")
.renderingMode(.template)
.resizable()
.frame(width: 35, height: 35)
.foregroundColor(place.show ? .red : annotationColor)
.onTapGesture {
if !place.show {
tapOnMapAnnotation(
place: place
)
}
}
.overlay(
annotationIcon.renderingMode(.template).resizable().frame(width: 10, height: 8).foregroundColor(.white).padding(.bottom, 15)
)
}
Now if I can engage in a bit of mind-reading (always questionable territory), I suspect that your real motivation in wanting to unify those functions is to make it easier to maintain the attributes, maybe adding new categories. If that's the case, you could eliminate the switch entirely, and use a global (or static) constant Dictionary:
fileprivate let annotationAttributes: [String: (iconName: String, rgbHex: String)] =
[
"Route" : ("car-side", "#6A798E"),
"Scenic Area": ("camera", "#006994"),
"Attraction" : ("people-group", "#00954C"),
"Hospitality": ("bed", "#6D5B97"),
"Landmark" : ("landmark-flag", "#5D5639"),
]
private func mapAnnotationAttributes(_ category: String) -> (Image, Color)
{
let (iconName, rgbHex) = annotationAttributes[category]
?? ("NA", "#FFFFFF")
let icon = Image(iconName)
let color = Color.init(UIColor(hexString: rgbHex))
return (icon, color)
}
Also if the icon image is only ever modified by the properties you specify in viewForPlace, you could apply them in mapAnnotationAttributes and simpifly the calling code even more. But if you want to re-use it with, for example, different foreground color, or whatever, it might better to keep the application of those modifiers in the caller.
Others suggested creating an enum for your category. Although I chose not to do that up to this point, staying as close as possible to your data representations, I do agree that's a good suggestion:
enum Category: String, Hashable
{
case route = "Route"
case scenicArea = "Scenic Area"
case attraction = "Attraction"
case hospitality = "Hospitality"
case landmark = "Landmark"
}
fileprivate let annotationAttributes: [Category: (iconName: String, rgbHex: String)] =
[
.route : ("car-side", "#6A798E"),
.scenicArea : ("camera", "#006994"),
.attraction : ("people-group", "#00954C"),
.hospitality: ("bed", "#6D5B97"),
.landmark : ("landmark-flag", "#5D5639"),
]
private func mapAnnotationAttributes(_ category: Category) -> (Image, Color)
{
let (iconName, rgbHex) = annotationAttributes[category]
?? ("NA", "#FFFFFF")
let icon = Image(iconName)
let color = Color.init(UIColor(hexString: rgbHex))
return (icon, color)
}
The up-side, and it's a big up-side, in my opinion, is the type-safety you get from having a distinct type for Category rather than a String that could be anything.
The down-side is that now you're back to maintaining two pieces of code when you want to add a new category: one for the category itself, and one for the mapping to icons and colors, and that's true whether you use a switch or a Dictionary. It might seem better to use a switch in this case because it enforces that you cover all the cases... except that using a default case kind of prevents that, so you'd need to get rid of the default case. You want the compiler error when you don't cover all the cases explicitly.
It comes down to who is in control of defining categories.
If it's your code, then what you want is an enum with a switch instead of a Dictionary and no default case. That way, when you add a new category, the compiler will inform you of all the places you need to update switch statements. Hopefully it's just one switch, but maybe at some point you do some non-UI stuff based on category. Then you'd have another switch somewhere.
On the other hand, if a server is in control of defining the categories, and assuming you have to do something reasonable when it eventually sends one your code doesn't know about, staying with String is quite reasonable, though please define some constants for them, and use them instead of string literals in your code to reduce opportunities for typos to creep in. In this case you can still use the Dictionary - though you might want to assert or at least log when you get an unknown category. You could still use an enum, and at least get some of the type-safety benefits, but you'd need to define a case to handle unknown category strings from the server. That doesn't help you much with maintenance in terms of knowing about new categories from the server, but because something like case unknown is a specific case, when you do discover there are new categories you need to handle, once you add them to the enum, at least you'll get compiler errors for all corresponding switch statements that need updating.