11:05 a.m.
On a Thursday in 2021, Kristina Hanicova wrote:
This patch includes small refactoring such as:
* early return in case of an error - helps with indentation
* removal of 'else' branch after return - unnecessary
* altering code to be more consistent with the rest of the file -
function calls inside of parentheses, etc.
* removal of unnecessary variables - mainly the ones used for
return value instead of returning it directly
* missing parentheses around multi-line block of code
There's too much going on in one patch at the same time.
Please either do one kind of change in a patch, or split it
per-function. And changes that result in moving lots of lines
to a different indentation level are easier to read if they are
separate from other changes.
Signed-off-by: Kristina Hanicova <khanicov(a)redhat.com>
---
tools/virsh-checkpoint.c | 19 +--
tools/virsh-domain-monitor.c | 314 ++++++++++++++++-------------------
2 files changed, 155 insertions(+), 178 deletions(-)
diff --git a/tools/virsh-checkpoint.c b/tools/virsh-checkpoint.c
index 8ad37ece69..c1a9e66a24 100644
--- a/tools/virsh-checkpoint.c
+++ b/tools/virsh-checkpoint.c
@@ -931,13 +931,13 @@ cmdCheckpointParent(vshControl *ctl,
if (virshGetCheckpointParent(ctl, checkpoint, &parent) < 0)
return false;
+
if (!parent) {
vshError(ctl, _("checkpoint '%s' has no parent"), name);
return false;
}
vshPrint(ctl, "%s", parent);
-
I was asked to leave an empty line before the final return by a reviewer
recently, so it seems some people like it.
return true;
}
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index eb3e0ef11a..d9bc869080 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -58,6 +58,7 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool
title,
g_autoptr(xmlDoc) doc = NULL;
g_autoptr(xmlXPathContext) ctxt = NULL;
int type;
+ int errCode;
if (title)
type = VIR_DOMAIN_METADATA_TITLE;
@@ -66,19 +67,18 @@ virshGetDomainDescription(vshControl *ctl, virDomainPtr dom, bool
title,
if ((desc = virDomainGetMetadata(dom, type, NULL, flags))) {
return desc;
- } else {
- int errCode = virGetLastErrorCode();
-
- if (errCode == VIR_ERR_NO_DOMAIN_METADATA) {
- desc = g_strdup("");
- vshResetLibvirtError();
- return desc;
- }
+ }
+ errCode = virGetLastErrorCode();
- if (errCode != VIR_ERR_NO_SUPPORT)
- return desc;
+ if (errCode == VIR_ERR_NO_DOMAIN_METADATA) {
+ desc = g_strdup("");
+ vshResetLibvirtError();
+ return desc;
You can return g_strdup("") directly.
}
+ if (errCode != VIR_ERR_NO_SUPPORT)
+ return desc;
Another alternative could be to split the fallback code below into a
separate function and call it if errCode == NO_SUPPORT
+
/* fall back to xml */
if (virshDomainGetXMLFromDom(ctl, dom, flags, &doc, &ctxt) < 0)
return NULL;
@@ -467,79 +467,72 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
The changes in cmdDomblkinfo deserve their own patch.
human = vshCommandOptBool(cmd, "human");
- if (all) {
- bool active = virDomainIsActive(dom) == 1;
- int rc;
+ if (!all) {
[...]
@@ -584,8 +577,6 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "inactive"))
flags |= VIR_DOMAIN_XML_INACTIVE;
- details = vshCommandOptBool(cmd, "details");
-
if (virshDomainGetXML(ctl, cmd, flags, &xmldoc, &ctxt) < 0)
return false;
@@ -593,7 +584,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
if (ndisks < 0)
return false;
- if (details)
+ if ((details = vshCommandOptBool(cmd, "details")))
I prefer the version where we process the command options first and
initialize the bool separately.
table = vshTableNew(_("Type"),
_("Device"), _("Target"), _("Source"), NULL);
else
table = vshTableNew(_("Target"), _("Source"), NULL);
@@ -618,8 +609,7 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
}
}
- target = virXPathString("string(./target/@dev)", ctxt);
- if (!target) {
+ if (!(target = virXPathString("string(./target/@dev)", ctxt))) {
vshError(ctl, "unable to query block list");
return false;
}
@@ -648,19 +638,16 @@ cmdDomblklist(vshControl *ctl, const vshCmd *cmd)
"|./source/@path)", ctxt);
}
- if (details) {
- if (vshTableRowAppend(table, type, device, target,
- NULLSTR_MINUS(source), NULL) < 0)
- return false;
- } else {
- if (vshTableRowAppend(table, target,
- NULLSTR_MINUS(source), NULL) < 0)
- return false;
- }
+ if (details && (vshTableRowAppend(table, type, device, target,
+ NULLSTR_MINUS(source), NULL) < 0))
+ return false;
+
+ if (!details && (vshTableRowAppend(table, target,
+ NULLSTR_MINUS(source), NULL) < 0))
+ return false;
The original is easier to read to me - the else branch is more visible
than the exclamation mark.
}
vshTablePrintToStdout(table, ctl);
-
return true;
}
@@ -808,16 +795,15 @@ cmdDomIfGetLink(vshControl *ctl, const vshCmd *cmd)
return false;
}
- if (ninterfaces < 1) {
- if (macstr[0])
+ if (ninterfaces != 1) {
+ if (ninterfaces > 1)
It's more readable to me to duplicate the 'return else' rather than the
condition.
In the spirit of shorter if's than elses
[ https://libvirt.org/coding-style.html#curly-braces ]
you can simply move the n > 1 condition above n < 1
+ vshError(ctl, _("multiple matching interfaces
found"));
+ else if (macstr[0])
vshError(ctl, _("Interface (mac: %s) not found."), macstr);
else
vshError(ctl, _("Interface (dev: %s) not found."), iface);
return false;
- } else if (ninterfaces > 1) {
- vshError(ctl, _("multiple matching interfaces found"));
- return false;
}
ctxt->node = interfaces[0];
Jano