3
\$\begingroup\$

For a job in company I had to do a code assignment. My assignment performs all the required tasks needed to be done. But after one month the company provided me a feedback and said that we have reviewed your exercise, very sorry for the delay. Unfortunately, while working, the structure and readability of the code are not sufficient, and we will not be proceeding further with your application at this time. The assignment was not to make a big application with a lot of modules. I am not able to figure out how can I improve it. Below is the assignment and my code, please review it and provide me your suggestions. Thanks

enter image description here enter image description here enter image description here enter image description here

 import SwiftUI
struct ContentView : View {
@State private var selectedBlocks = [[String:Int]]()
@State private var scoreArray = [[String:Int]]()
@State private var finalScore = ""
@State private var isUserInteractionDisabled = false
private let emptyNeighborScore = 10
private let filledNeighborScore = 5
private let intialBlockScore = 5
private let numberOfColumns = 5
private let numberOfRows = 5
private var resultLength = 10
private var columns: [GridItem] {
Array(repeating: .init(.adaptive(minimum: 60)), count: numberOfColumns)
}
var body: some View {
 HStack(spacing: 0){
 Text("Score: \(finalScore )")
 }.frame(height: 100)
 
ScrollView(.vertical, showsIndicators: false) {
 ForEach(0..<numberOfRows) { rowIndex in
 LazyVGrid(columns: columns, spacing: 20) {
 ForEach(0..<numberOfColumns) { columnIndex in
 let cellIndex: [String:Int] = ["rowIndex":rowIndex, "columnIndex":columnIndex
 ]
 let results = self.scoreArray.filter { 0ドル["rowIndex"] == rowIndex && 0ドル["columnIndex"] == columnIndex
 }
 Button(action: {
 self.updateSelectBlock(rowIndex:rowIndex, columnIndex:columnIndex) // <- on tap update selection
 }) {
 
 Text(results.count>0 ? "\(results[0]["score"] ?? 0 )" : " ")
 }
 .frame(width:50)
 .border(Color.black, width: 2)
 .background(self.selectedBlocks.contains(cellIndex) ? Color.blue : Color.white)
 }
 .buttonStyle(PlainButtonStyle())
 }
 }
}.disabled(isUserInteractionDisabled)
}
func updateSelectBlock(rowIndex:Int, columnIndex:Int) {
 isUserInteractionDisabled = true
 var cellIndex: [String:Int] = ["rowIndex":rowIndex, "columnIndex":columnIndex
 ]
 
 if(!self.selectedBlocks.contains(cellIndex) && self.selectedBlocks.count < resultLength) {
 self.selectedBlocks.append(cellIndex)
 }
 else {
 return
 }
 
 if(rowIndex < numberOfRows-1) {
 let rowDifference = (numberOfRows - rowIndex - 2)
 var currentRowIndex = rowIndex
 var hasNeigbhor = false
 var count = 0
 _ = Timer.scheduledTimer(withTimeInterval: 0.1, repeats: true){ t in
 if count == (rowDifference) {
 t.invalidate()
 isUserInteractionDisabled = false
 }
 count += 1
 let bottomNeighborCheck = checkNeigbhor(rowIndex:currentRowIndex+1, columnIndex:columnIndex)
 let horizontalNeigborsCheck = checkHorizontalNeigbhors(rowIndex:currentRowIndex, columnIndex:columnIndex)
 let activeNeighborsCheck = horizontalNeigborsCheck || bottomNeighborCheck
 if(!activeNeighborsCheck) {
 if let index = self.selectedBlocks.firstIndex(of: cellIndex) {
 self.selectedBlocks.remove(at: index)
 }
 currentRowIndex = currentRowIndex + 1
 cellIndex = ["rowIndex":currentRowIndex, "columnIndex":columnIndex]
 if(!self.selectedBlocks.contains(cellIndex)) {
 self.selectedBlocks.append(cellIndex)
 }
 }
 else {
 if(!self.selectedBlocks.contains(cellIndex) && hasNeigbhor == false) {
 if let index = self.selectedBlocks.firstIndex(of: cellIndex) {
 self.selectedBlocks.remove(at: index)
 }
 hasNeigbhor = true
 self.selectedBlocks.append(cellIndex)
 }
 }
 showScore(checkActiveNeighbors:activeNeighborsCheck, currentRowIndex:currentRowIndex)
 }
 }
}
func checkNeigbhor(rowIndex:Int, columnIndex:Int) -> Bool {
 let leftNeighborIndex : [String:Int] = ["rowIndex":rowIndex, "columnIndex":columnIndex]
 return self.selectedBlocks.contains(leftNeighborIndex) ? true :false
}
func checkHorizontalNeigbhors(rowIndex:Int, columnIndex:Int) -> Bool {
 let leftNeighborCheck = checkNeigbhor(rowIndex:rowIndex, columnIndex:columnIndex-1)
 let rightNeighborCheck = checkNeigbhor(rowIndex:rowIndex, columnIndex:columnIndex+1)
 return leftNeighborCheck && rightNeighborCheck
}
func showScore(checkActiveNeighbors:Bool, currentRowIndex:Int) {
 if( self.selectedBlocks.count == resultLength && (checkActiveNeighbors || currentRowIndex == numberOfRows-1)) {
 self.calculateScore()
 return
 }
}
func calculateScore() {
 for blockIndex in 0..<self.selectedBlocks.count {
 var score = intialBlockScore
 var rowIndexOfBlock : Int = self.selectedBlocks[blockIndex]["rowIndex"] ?? 0
 let columnIndexOfBlock : Int = self.selectedBlocks[blockIndex]["columnIndex"] ?? 0
 
 if(rowIndexOfBlock < numberOfRows) {
 let rowDifference = (numberOfRows - rowIndexOfBlock - 1)
 for n in 0..<rowDifference {
 let localIndex = rowIndexOfBlock+n
 let bottomNeighborCheck = checkNeigbhor(rowIndex:localIndex+1, columnIndex:columnIndexOfBlock)
 
 if(!bottomNeighborCheck && localIndex+1<numberOfRows){
 let cellIndex = ["rowIndex":localIndex+1, "columnIndex":columnIndexOfBlock, "score":emptyNeighborScore]
 if(!self.scoreArray.contains(cellIndex)) {
 self.scoreArray.append(cellIndex)
 }
 }
 else if (bottomNeighborCheck && rowIndexOfBlock+1<numberOfRows) {
 score = score + filledNeighborScore
 }
 }
 let cellIndex = ["rowIndex":rowIndexOfBlock, "columnIndex":columnIndexOfBlock, "score":score]
 if(!self.scoreArray.contains(cellIndex)) {
 self.scoreArray.append(cellIndex)
 }
 rowIndexOfBlock = rowIndexOfBlock + 1
 }
 calculateFinalScore()
 }
}
func calculateFinalScore() {
 var totalScore = 0
 for inde in 0..<self.scoreArray.count {
 let blockScore : Int = self.scoreArray[inde]["score"] ?? 0
 totalScore = totalScore+blockScore
 }
 finalScore = " \(totalScore )"
}
}
struct ContentView_Previews: PreviewProvider {
 static var previews: some View {
 ContentView()
 }
}
 
asked Jun 23, 2021 at 12:45
\$\endgroup\$
2
  • \$\begingroup\$ The over arching issue I see is a lack of separation of concerns. You have game logic in your view. \$\endgroup\$ Commented Jun 25, 2021 at 2:56
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ Commented Aug 9, 2021 at 6:40

1 Answer 1

3
\$\begingroup\$

First there's some low hanging fruit. Perpetual use of self. when not needed. Use of parens when not needed. Inconsistent formatting (sometimes you put a space before the open brace, sometimes you don't. Sometimes you put code after a close brace on the same line, sometimes you don't.)

These seem like minor things I'm sure and the compiler doesn't care at all, but it's a sign of sloppy thinking, rushed code, or someone who just doesn't care. You want it to look like you care, didn't do a rush job and think clearly.

That single, random comment in the code that does nothing but point out the obvious is a red flag as well. If you are going to put comments in, they should explain why you did what you did, not describe what the code is doing. Anybody who can read Swift will know what the code is doing, but they won't know why you did it unless you tell them.

The super multi-purpose scoreArray = [[String: Int]]() is an odd choice. It looks like each [String: Int] represents something that has a rowIndex and columnIndex and a score. Maybe use a struct for that instead of a Dictionary.

And as Alexander said in the comments, the lack of separation between logic and view is the biggest problem. As the code sits, it's impossible to test.

Some examples, if I was reviewing this code, I would want to see a function that takes in a board state and returns a score. I would want to see a function that takes a board state and a coordinate and returns a Bool indicating whether a block at that location would need to drop. Others like that as well. I would want to see a type that represents the BoardState (maybe even called BoardState)

Even if you didn't write tests, even if you weren't tasked to write tests, your code should be testable.

answered Aug 8, 2021 at 22:37
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.