Task 02: git task by marynamalakhova · Pull Request #92 · Kernel-GL-HRK/gl-kernel-training-2021

marynamalakhova · 2021-03-10T19:13:51Z

Add game "scissors". Used matrix for getting decision to find the winner.

AleksandrBulyshchenko · 2021-03-11T15:10:56Z

@marynamalakhova, what's the purpose of picking 12e9e40 (as 9cdd359) instead of basing the branch on top of bd00000?

AleksandrBulyshchenko · 2021-03-11T15:41:20Z

Please use enums instead of digits and separate resources (matrix, strings, etc.) from logic (define in different source files).

marynamalakhova · 2021-03-11T18:46:44Z

Opened another PR

AleksandrBulyshchenko

@marynamalakhova,
Please see my comments below.

Also please note that the fixes should be made inplace (in the patches where the issues are appear).

scissors/scissors_game/main.c

AleksandrBulyshchenko · 2021-03-22T16:51:39Z

scissors/scissors_game/scissors.h

+const int game_table[3][3]={
+       eDRAW, eLOST, eWINN,
+       eWINN, eDRAW, eLOST,
+       eLOST, eWINN, eDRAW
+};


You shouldn't define variables in header files.
This will lead to error in case of it's inclusion in more than one source file (which is actually what headers are for).
The table should be moved into separate source file.
Together with function for comparison (and probably function for parsing as well).

marynamalakhova · 2021-03-24T10:17:19Z

Thank you for review. I've fixed

AleksandrBulyshchenko

Current separation into modules is mostly meaningful.
(All the code is actually moved together and main just call single function in a loop.)
Please provide real separation:

all input / output should be separated from the game logic (e.g. remain in the main module);
game logic should implement separate functions for:
- user value (transform user input into enum);
- comp value (returns randomly generated enum);
- result calculation (gets two values and returns a result;

Also see few comments inline.

scissors/scissors_game/scissors.c

scissors/scissors_game/scissors.h

scissors/scissors_game/scissors.c

marynamalakhova · 2021-04-02T08:58:40Z

Thank you for review. I`ve already fixed.

Add main file with initial output (according to task02). Add Makefile. Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

AleksandrBulyshchenko · 2021-04-05T20:23:46Z

scissors/scissors_game/scissors.h

+static const char * const Game_Elements[] = {
+	"unknown", "rock", "paper", "scissors"};
+static enum {
+	UNKNOWN,
+	ROCK,
+	PAPER,
+	SCISSORS
+} element_id;


Variable definitions (and thus - memory allocation) in header!

What's the purpose of element_id at all? - Most probably you've meant something different.

AleksandrBulyshchenko · 2021-04-05T20:25:05Z

scissors/scissors_game/scissors.c

+	case 's':
+		ans = SCISSORS;
+	}


Case without break - high probability of error in case of extention.

AleksandrBulyshchenko · 2021-04-05T20:29:30Z

scissors/scissors_game/main.c

+		res = char_to_element_index(input);
+		if (!res)
+			printf("You've chosen something strange!\n");


char_to_element_index() in case of error returns UNKNOWN, but return value is compared to 0.

Currently UNKNOWN == 0, but this isn't guaranteed in case of modification. If return values have exact meaning, it's comparison should be explicit.

AleksandrBulyshchenko · 2021-04-05T20:32:47Z

scissors/scissors_game/scissors.h

+	PAPER,
+	SCISSORS
+} element_id;
+int play_scissors(void);


play_scissors() is declared in 5ea78fe, but not defined.

AleksandrBulyshchenko · 2021-04-05T20:34:20Z

scissors/scissors_game/scissors.c

+int play_scissors(void)
+{
+	return (rand()%3)+1;
+}


Definitions order in source file and declarations order in header should be aligned.

AleksandrBulyshchenko · 2021-04-05T20:35:42Z

scissors/scissors_game/main.c

+		case DRAW:
+			printf("It's draw\n");
+		}


Case without break!

AleksandrBulyshchenko · 2021-04-05T20:37:38Z

scissors/scissors_game/scissors.c

+/*		rock	paper	scissors	*/
+/* rock*/	DRAW,   LOST,  WINN,
+/* paper*/	WINN,   DRAW,  LOST,
+/* sciss*/	LOST,   WINN,  DRAW


Please whenever possible avoid mixing tabs and spaces for indentation.

AleksandrBulyshchenko · 2021-04-05T20:39:05Z

scissors/scissors_game/scissors.c

+
+int get_game_result(int user, int comp)
+{
+	return game_table[user-1][comp-1];


Using enum instead of 1 will make code more robust.

AleksandrBulyshchenko · 2021-04-05T20:40:15Z

scissors/scissors_game/scissors.h

+static enum {
+	DRAW,
+	LOST,
+	WINN
+} game_results;


What's the purpose of variable game_results? - Most probably you've meant something different.

Add users input reading and printing if it is a wrong one Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

Init random. Add new random decision of the computer's choise. Finish intermidiate output. Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

Add table an function in order to get the winner Add game result output Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

marynamalakhova · 2021-04-06T07:49:49Z

Thank you for review, I`ve fixed.

marynamalakhova added the Ready for review Ready for review label Mar 10, 2021

marynamalakhova requested a review from AleksandrBulyshchenko March 10, 2021 19:13

marynamalakhova force-pushed the task_scissors branch 2 times, most recently from 011ef33 to a2a404c Compare March 11, 2021 17:25

marynamalakhova closed this Mar 11, 2021

marynamalakhova reopened this Mar 11, 2021

marynamalakhova force-pushed the task_scissors branch from 214ccac to 8bbc515 Compare March 11, 2021 19:52

AleksandrBulyshchenko mentioned this pull request Mar 11, 2021

02_git_task. Scissors game #96

Closed

AleksandrBulyshchenko reviewed Mar 22, 2021

View reviewed changes

AleksandrBulyshchenko added Change requested Change requested and removed Ready for review Ready for review labels Mar 22, 2021

marynamalakhova force-pushed the task_scissors branch 5 times, most recently from 8dfc498 to e237ba7 Compare March 24, 2021 10:15

marynamalakhova added the Ready for review Ready for review label Mar 24, 2021

marynamalakhova removed the Change requested Change requested label Mar 25, 2021

AleksandrBulyshchenko reviewed Apr 2, 2021

View reviewed changes

AleksandrBulyshchenko added Change requested Change requested and removed Ready for review Ready for review labels Apr 2, 2021

marynamalakhova force-pushed the task_scissors branch 2 times, most recently from 220cce0 to 46cf741 Compare April 2, 2021 08:55

marynamalakhova added Ready for review Ready for review and removed Change requested Change requested labels Apr 2, 2021

SCISSORS: Add Makefile and main

d8ec6b1

Add main file with initial output (according to task02). Add Makefile. Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

marynamalakhova force-pushed the task_scissors branch 2 times, most recently from 7f85ddb to 64021cb Compare April 5, 2021 13:24

AleksandrBulyshchenko requested changes Apr 5, 2021

View reviewed changes

AleksandrBulyshchenko added Change requested Change requested and removed Ready for review Ready for review labels Apr 5, 2021

marynamalakhova force-pushed the task_scissors branch 2 times, most recently from 1678de6 to 28ece96 Compare April 6, 2021 07:43

marynamalakhova added 3 commits April 6, 2021 10:45

SCISSORS: Add reading users input and it`s convertion to text

42eaa77

Add users input reading and printing if it is a wrong one Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

SCISSORS: Add random choise of the computers' decision

dcf92da

Init random. Add new random decision of the computer's choise. Finish intermidiate output. Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

SCISSORS: Add game and Finish task

889a3cc

Add table an function in order to get the winner Add game result output Signed-off-by: Maryna Malakhova <maryna.malakhova@globallogic.com>

marynamalakhova force-pushed the task_scissors branch from 28ece96 to 889a3cc Compare April 6, 2021 07:47

marynamalakhova added Ready for review Ready for review and removed Change requested Change requested labels Apr 6, 2021

Conversation

marynamalakhova commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AleksandrBulyshchenko commented Mar 11, 2021

Uh oh!

AleksandrBulyshchenko commented Mar 11, 2021

Uh oh!

marynamalakhova commented Mar 11, 2021

Uh oh!

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marynamalakhova commented Mar 24, 2021

Uh oh!

AleksandrBulyshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marynamalakhova commented Apr 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marynamalakhova commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marynamalakhova commented Mar 10, 2021 •

edited

Loading