Code review

A Sodipedia wikiből

A code review az az informatikai kifejezés, amikor egy rutinosabb, senior programozó átnézi, ellenőrzi egy kezdőbb programozó kódját az élesítés előtt. Informatikai cégeknél bejáratott módszer, egyszersmind jó visszajelzések ezek a tapasztalatlanabb kollégáknak, mit rontottak el, mire kell figyelniük legközelebb, miben kell fejlődniük.

Endre retteg a code review-któl, neki a legjobb, ha senki által nem ellenőrizve, számonkérve alibizhet eldugva a sarokba, 2 órás feladatokon molyolva hetekig. Amikor code review-t kap, azok rövid úton rávilágítanak alkalmatlanságára, aminek az (ismételt) kirúgás a nyilvánvaló eredménye.


Alább egy code review egy cégtől, akiknél próbafeladatot csinált. Talán nem lepődik meg senki rajta, de valamiért nem vették fel...


Hallo Endre,

vielen Dank für das entsprechende Umsetzen!

Auf Basis der zugesandten Unterlagen müssen wir dir leider mitteilen, dass du nicht in den weiteren Auswahlprozess kommst.

Mit deiner angegebenen Erfahrung und den Gehaltsvorstellungen passen die Erwartungen von uns an fachliches Knowhow und entsprechende Umsetzungen nicht zusammen.


Im Folgenden eine Sammlung von konkretem Feedback zur Implementierung, damit du auch weißt, welche genauen Gründe aus fachlicher Sicht dahinterstehen:


Visual flaws:

  • Page load of about 7 Seconds until you see more than a headline. The graphql request takes about 5-6 seconds of that but there is no loading indicator.
  • No cursor pointer or click indicator on list items
  • Very minimalist styling
  • No images in detail view (which were requested in the assignment!)
  • If you put links in detail view, they should at least be clickable. They are not.
  • null and undefined printed in the detail view


Implementation flaws:

  • Too much data in the initial graphql request, all detail in the first request and no pagination (so this is the only request ever sent) - pagination is only in the frontend. (This causes long initial wait)
  • String concatenations instead of template literals (ES6 basics!)
  • Mixing of ' and " for strings in code - uniformity!
  • Not making use of basic react functionality but setting detail view with ...
  • .innerHTML = ...
  • Not using npm, no package.json (was not requested, but should be self-evident)
  • Spaghetti-Code, all in one single file
  • Not using any library for graphql, but writing the query as string-concatenation
  • no proper error handling in graphql request
  • console logs not removed - code style!
  • No indentations in surrounding HTML frame - code style!
  • Missing semicolon (line 289) - code style!
  • No linting / test cases (were not requested but should at least be mentioned as "To be considered before release" - which was requested but is completely missing from the readme)


Vielen Dank und alles Gute für deine weitere berufliche Zukunft.

Viele Grüße


A legfigyelemreméltóbb megállapítások:

  • Mit deiner angegebenen Erfahrung und den Gehaltsvorstellungen passen die Erwartungen von uns an fachliches Knowhow und entsprechende Umsetzungen nicht zusammen - A megadott tapasztalatok és a fizetési elvárások nem állnak össze a szakmai tudással és a megvalósítás megfelelőségével. Röviden: ne hazudj a CV-dben, te kókler gánymester!!!
  • kimaradtak olyan megvalósítások, amik direkt le voltak írva az elvárások között.
  • nem megfelelő megvalósítások alkalmazása, ami nem volt külön leírva, de szakmailag magától értetődő lenne.
  • If you put links in detail view, they should at least be clickable. They are not. - ha linkeket teszel a részletes nézetben, legalább klikkelhetők lennének. Mert hogy nem azok. Enyhén szólva cinikus kritika magától értetődő dolgokra.
  • minimalista, primitív stílus, spagetti-kód. Minden áttekinthetetlenül, karbantarthatatlanul egybe van gányolva, nincsen stílusosan, áttekinthető módon felépítve a kód.


Informatikus snecológusok már számtalanszor kritizálták Endre programozói felkészületlenségét, alkalmatlanságát. De ilyen esetekben, amikor maga a cég vagy az ügyfél (lásd: Krizsai Andrea) ad nyilvánvaló visszajelzést alkalmatlanságáról, a valóság is átüt az auti-páncélon.


A kérdéses próbafeladat megörökítve az utókornak:

<!DOCTYPE html>
<html>
<head>
    <meta charset="UTF-8" />
    <title>GraphiQL by Endre F. Kiss</title><style>
    th.MuiTableCell-head {
        font-weight: bold;
    }
    tr {
        height: 30px;
    }
    table.detailTable {
        font-family: Arial;
    }
</style>
</head>
<body>

<h2>Launches</h2>

<script src="https://unpkg.com/react@16.13.1/umd/react.development.js" crossorigin></script>
<script src="https://unpkg.com/react-dom@16.13.1/umd/react-dom.development.js" crossorigin></script>
<script src="https://unpkg.com/@material-ui/core@latest/umd/material-ui.production.min.js"></script>
<script src="https://unpkg.com/babel-standalone@latest/babel.min.js" crossorigin="anonymous"></script>

<div id="root"></div>
<script type="text/babel">
    const {
        colors,
        ThemeProvider,
        makeStyles,
        Table,
        TableBody,
        TableCell,
        TableContainer,
        TableHead,
        TableRow,
        TablePagination,
        Paper,
        Grid,
        withStyles,
        createStyles,
        createMuiTheme,
    } = MaterialUI;

    const theme = createMuiTheme({
        palette: {
            primary: {
                main: '#556cd6',
            },
            secondary: {
                main: '#19857b',
            },
            error: {
                main: colors.red.A400,
            },
            background: {
                default: '#fff',
            },
        },
    });



    function App() {

        const useStyles = makeStyles({
            table: {
                minWidth: 650,
            }
        });

        const classes = useStyles();
        const [page, setPage] = React.useState(0);
        const [rowsPerPage, setRowsPerPage] = React.useState(10);

        const handleChangePage = (event: unknown, newPage: number) => {
            setPage(newPage);
        };

        const handleChangeRowsPerPage = (event: React.ChangeEvent<HTMLInputElement>) => {
            setRowsPerPage(parseInt(event.target.value, 10));
            setPage(0);
        };

        const StyledTableRow = withStyles((theme: Theme) =>
            createStyles({
                root: {
                    '&:nth-of-type(odd)': {
                        backgroundColor: theme.palette.action.hover,
                    },
                },
            }),
        )(TableRow);

        let row2 = {};

        const showDetails = (row) => (event: React.MouseEvent<unknown>) => {

            let tableContent = ''
                +'<table class="detailTable">'
                + '<tbody>'
                + '<tr>'
                +    '<th style="width: 130px">Name</th>'
                +    '<td>' + row.mission_name + '</td>'
                + '</tr>'
                + '<tr>'
                +    '<th>Launch date</th>'
                +    '<td>' + row.launch_date_local + '</td>'
                + '</tr>'
                + '<tr>'
                +    '<th>Launch site</th>'
                +    '<td>' + row.launch_site.site_name_long + '</td>'
                + '</tr>'
                + '<tr>'
                +    '<th>Article link</th>'
                +    '<td>' + row.links.article_link + '</td>'
                + '</tr>'
                + '<tr>'
                +    '<th>Video link</th>'
                +    '<td>' + row.links.video_link + '</td>'
                + '</tr>'
                + '<tr>'
                +    '<th>Rocket</th>'
                +    '<td>' + row.rocket.rocket_name + '</td>'
                + '</tr>';

            let shipNumber = 1;
            row.ships = row.ships.filter(x => x !== null);
            row.ships.map((ship) =>
                (
                    tableContent += ''
                        + '<tr>'
                        + '<th colspan="2" style="text-align: center">Ship #' + shipNumber++ + '</th>'
                        + '</tr>'
                        + '<tr>'
                        + '<th>Ship name</th>'
                        + '<td>' + ship.name + '</td>'
                        + '</tr>'
                        + '<tr>'
                        + '<th>Home port</th>'
                        + '<td>' + ship.home_port + '</td>'
                        + '</tr>'
                ))

            let payloadNumber = 1;
            console.log(row);
            row.rocket.second_stage.payloads = row.rocket.second_stage.payloads.filter(x => x !== null);

            row.rocket.second_stage.payloads.map((payload) =>
                (
                    tableContent += ''
                        + '<tr>'
                        + '<th colspan="2" style="text-align: center">Payload #' + payloadNumber++ + '</th>'
                        + '</tr>'
                        + '<tr>'
                        + '<th>Payload type</th>'
                        + '<td>' + payload.payload_type + '</td>'
                        + '</tr>'
                        + '<tr>'
                        + '<th>Payload mass kg</th>'
                        + '<td>' + payload.payload_mass_kg + '</td>'
                        + '</tr>'
                        + '<tr>'
                        + '<th>Payload mass lps</th>'
                        + '<td>' + payload.payload_mass_lps + '</td>'
                        + '</tr>'
                ))

            tableContent += ''
                + '</tbody></table>';

            document.getElementById("kutya").innerHTML = tableContent;
        };

        return (
            <div style={{ height: 400, width: '100%' }}>
                <Grid container spacing={3}>
                    <Grid item xs={12} sm={6}>
                        <TableContainer component={Paper}>
                            <Table className={classes.table} aria-label="simple table">
                                <TableHead>
                                    <TableRow>
                                        <TableCell>Name</TableCell>
                                        <TableCell align="right">Launch date</TableCell>
                                        <TableCell align="right">Launch site</TableCell>
                                    </TableRow>
                                </TableHead>
                                <TableBody>
                                    {window.result.slice(page * rowsPerPage, page * rowsPerPage + rowsPerPage).map((row) => (
                                        <StyledTableRow onClick={showDetails(row)} key={row.mission_name}>
                                            <TableCell component="th" scope="row">
                                                {row.mission_name}
                                            </TableCell>
                                            <TableCell align="right">{row.launch_date_local}</TableCell>
                                            <TableCell align="right">{row.launch_site.site_name_long}</TableCell>
                                        </StyledTableRow>
                                    ))}
                                </TableBody>
                            </Table>
                        </TableContainer>
                        <TablePagination
                            rowsPerPageOptions={[10, 15]}
                            component="div"
                            count={window.result.length}
                            rowsPerPage={rowsPerPage}
                            page={page}
                            onChangePage={handleChangePage}
                            onChangeRowsPerPage={handleChangeRowsPerPage}
                        />
                    </Grid>
                    <Grid item xs={12} sm={6}>
                        <Paper id="kutya" className={classes.paper}>
                            <table>
                                <tbody>
                                <tr>
                                    <td>Name</td>
                                    <td>{row2.mission_name}</td>
                                </tr>
                                </tbody>
                            </table>
                        </Paper>
                    </Grid>
                </Grid>
            </div>
        );
    }

    const requestOptions = {
        method: 'POST',
        headers: { 'Content-Type': 'application/json' },
        body: JSON.stringify({ query:
                '{\n' +
                '  launchesPast(limit: 0) {\n' +
                '    mission_name\n' +
                '    launch_date_local\n' +
                '    launch_site {\n' +
                '      site_name_long\n' +
                '    }\n' +
                '    links {\n' +
                '      article_link\n' +
                '      video_link\n' +
                '    }\n' +
                '    rocket {\n' +
                '      rocket_name\n' +
                '      first_stage {\n' +
                '        cores {\n' +
                '          flight\n' +
                '          core {\n' +
                '            reuse_count\n' +
                '            status\n' +
                '          }\n' +
                '        }\n' +
                '      }\n' +
                '      second_stage {\n' +
                '        payloads {\n' +
                '          payload_type\n' +
                '          payload_mass_kg\n' +
                '          payload_mass_lbs\n' +
                '        }\n' +
                '      }\n' +
                '    }\n' +
                '    ships {\n' +
                '      name\n' +
                '      home_port\n' +
                '      image\n' +
                '    }\n' +
                '  }\n' +
                '}\n'
        })
    };

    fetch("https://api.spacex.land/graphql", requestOptions)
        .then(res => res.json())
        .then(
            (result) => {
                console.log(result.data.launchesPast);
                window.result=result.data.launchesPast;
                ReactDOM.render(
                    <ThemeProvider theme={theme}>
                        <App />
                    </ThemeProvider>,
                    document.querySelector('#root'),
                );
            },
            (error) => {
                alert(error);
            }
        )
</script>

</body>
</html>

Forrás: https://github.com/amalono/graphql