Plz help me to identify the error which I made in this proram coding

Question

1.00/5 (1 vote)

See more:

C++

#include <stdio.h>
int factorial(int i);

int factorial(int i){
    int x;
    for(x=1;x<i;x++){
        i=i*x;
    }
    return i;
}

int main(){
    int a;
    a=factorial(5)+10;
    printf("%d\n",a);
    0;
}

What I have tried:

I tried many times, the logic seems right but the code is not getting right answer

Posted 13-Jul-20 7:24am

Rowan Kasim

Updated 13-Jul-20 23:27pm

Richard Deeming

v2

Add a Solution

Comments

Rick York 13-Jul-20 15:49pm

The word is PLEASE.

Stefan_Lang 14-Jul-20 5:04am

Actually it's 'please'. Shouting would be inappropriate ;-)

4 solutions

Solution 3

A possibly 'saner' approach to the function

C

int factorial(int i)
{
    int x, fact;
    // 'i' contains the passed argument, no need to change it
    // 'fact' holds the result
    // 'x' is the looping variable
    for(fact = 1, x=2; x <= i; ++x)
    {
        fact *= x;
    }
    return fact;
}

Posted 13-Jul-20 20:15pm

CPallini

Comments

Stefan_Lang 14-Jul-20 5:18am

I agree, but I doubt this advice will help a lot without an explanation *why* this is saner.

(also, but that's just an IMHO, it would be even clearer if you declare that function argument const - that would make it impossible to abuse it as a local variable)

Solution 4

Based on the insight offered in solution 1, the easiest fix would be inverting your for loop:

C++

for (int x = i-1; x > 1; --x)
   i *= x;

But, honestly, I would advise against using the input as a local, nodifiable variable. Solution 3 is much better in that regard: it doesn't modify i, and therefore doesn't run into the problem you had.

As a rule of thumb: do not modify function inputs, unless that is what the function is supposed to do. Declaring an extra variable only requires a trivial amount of typing, but it will save you a ton of time for not having to search for some obscure bugs like this one.

An even more general rule: the more verbose you are in writing your code, the harder it will get for bugs to hide. So don't shy away from long names or additional lines of code, if that makes your code easier to read and understand.

Posted 13-Jul-20 23:27pm

Stefan_Lang

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Patrice T · Accepted Answer · 2020-07-13T07:32:00

C++

int factorial(int i){
    int x;
    for(x=1;x<i;x++){ // i is the limit to end of loop
        i=i*x; // but here you change i
    }
    return i;
}

Quote:
Plz help me to identify the error which I made in this proram coding

Your code do not behave the way you expect, or you don't understand why !

There is an almost universal solution: Run your code on debugger step by step, inspect variables.
The debugger is here to show you what your code is doing and your task is to compare with what it should do.
There is no magic in the debugger, it don't know what your code is supposed to do, it don't find bugs, it just help you to by showing you what is going on. When the code don't do what is expected, you are close to a bug.
To see what your code is doing: Just set a breakpoint and see your code performing, the debugger allow you to execute lines 1 by 1 and to inspect variables as it execute.

Debugger - Wikipedia, the free encyclopedia[^]

Mastering Debugging in Visual Studio 2010 - A Beginner's Guide[^]
Basic Debugging with Visual Studio 2010 - YouTube[^]

1.11 — Debugging your program (stepping and breakpoints) | Learn C++[^]

The debugger is here to only show you what your code is doing and your task is to compare with what it should do.

Richard Deeming · Accepted Answer · 2020-07-13T07:32:00

Every time through your for loop, you're increasing the end-point of the loop (i). That loop will never end.

Stop and think about what you're actually trying to do. To calculate a factorial, you need to multiply together all of the numbers between 1 and i.

You'll need a separate variable to hold the result so that you're not overwriting i. That variable should initially be set to 1 before starting the loop.

Plz help me to identify the error which I made in this proram coding

4 solutions

Solution 1

Solution 2

Solution 3

Solution 4

Add your solution here

Preview 0