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()
     }
}


      
\$\endgroup\$
2
  • \$\begingroup\$ The over arching issue I see is a lack of separation of concerns. You have game logic in your view. \$\endgroup\$
    – Alexander
    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\$ Aug 9, 2021 at 6:40

1 Answer 1

2
\$\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.

\$\endgroup\$

Your Answer

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

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