I'm working on cleaning up a team members code. We're working on a code base for GUI testing and I assigned them to work on creating several of our basic action functions for "doing things" in the GUI. Their code works fine, but months later, I'm finally getting around to digging in really deep into the actual code itself (beyond checking basic formatting) and I'm realizing that one of the reasons it has always seemed so arcane to me, is that it's not extremely well written.
I heard a rule once, related writing clean and readable code, that your functions should only do one thing. I've been trying to break down some of these 6 or 7 layers of nested if statements into individual functions to reduce redundant code. I specifically came across one section that was repeated very often. It was an if statement, that would check if the object trying to be interacted with was visible or not. If it wasn't, it would run through a function to make it visible. Like this:
// classObject - an object in one of our classes
// sObject - an object that our IDE recognizes in the GUI and is a property of classObject
// nativeObject and .IsVisible - properties in sObject
// makeVisible - an array in the related class which contains objects needed to be clicked to make the object visible. Hence the forEach loop.
if (!object.exists(classObject.sObject) ||
(!waitForObjectExists(classObject.sObject).nativeObject.IsVisible &&
!(typeof classObject.makeVisible === "undefined" || classObject.makeVisible === "")))
{
try
{
classObject.makeVisible.forEach(function(tab) {
mouseClick(tab);
})
}
catch(e)
{
classObject.makeVisible.forEach(function(tab) {
snooze(0.1);
mouseClick(tab);
})
}
}
My first instinct, was just noticing that it was easy to turn into a function like makeVisibleFunction(). But then I realized a few things. 1) I had to devote all of my brain power to decipher the conditional and 2) that the entire block of code was firing only if this conditional was met, and there was no need for an else statement. I don't know for sure, but something tells me that it's not good for to have a function that only contains code wrapped in an if statement. I also didn't like the idea of having this same conditional repeated over and over again in every function needing to "makeVisible" the object. So I was wondering if I could enlist the internet geniuses to help me think of some ways to make this "cleaner". I don't want or mean to try and pass off my problem to others, but I'm kind of at a loss for where to go here.
Aucun commentaire:
Enregistrer un commentaire